-
Notifications
You must be signed in to change notification settings - Fork 979
Server side rendering support #2010
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Server side rendering support #2010
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2010 +/- ##
=======================================
Coverage 92.98% 92.98%
=======================================
Files 152 152
Lines 5926 5934 +8
Branches 1245 1247 +2
=======================================
+ Hits 5510 5518 +8
Misses 416 416
|
| */ | ||
| export function getEnv(): Required<ENVIRONMENT> { | ||
| const _window = window as typeof window & RAW_ENVIRONMENT; | ||
| const _window = (typeof window === 'undefined' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@obecny is there some reason not to use globalThis or similar? Maybe from core we should export something like
export const getGlobal = function () {
if (typeof globalThis !== 'undefined') { return globalThis; }
if (typeof self !== 'undefined') { return self; }
if (typeof window !== 'undefined') { return window; }
if (typeof global !== 'undefined') { return global; }
throw new Error('unable to locate global object');
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yah I think this type of check is a good idea. I didn't use globalThis because I wasn't sure if the browser support was there or not.
I can add a getGlobal function to core. Do you see any other places it's needed besides https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-core/src/platform/node/environment.ts#L29?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure if the browser support was there or not.
It's not there in IE at least. I would wait on feedback from @obecny before making changes to core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tbh I don't remember why it is not using global.
@ryhinchey there is already a property for that, but not in the core but in api _globalThis so just use this one.
If there is anything that needs to be changed in this _globalThis then feel free to open PR in api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@obecny sounds good - thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that I think about it, I'll break out the service worker support into a separate PR and keep this one just to server side rendering
cf60088 to
23cd68b
Compare
23cd68b to
4e7d96a
Compare
|
Is there anything else that needs to be done to land this? Thanks for the timely reviews! |
nope |
Which problem is this PR solving?
Running opentelemetry-web in a javascript application that's being server side renderedwill throw an error. This PR fixes that issue as documented here #1575
Short description of the changes