-
Notifications
You must be signed in to change notification settings - Fork 806
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
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
|
@@ -25,7 +25,9 @@ import { | |||
* Gets the environment variables | |||
*/ | |||
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