Skip to content
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

Merged
merged 4 commits into from
Mar 23, 2021

Conversation

ryhinchey
Copy link
Contributor

@ryhinchey ryhinchey commented Mar 11, 2021

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

  • moves the url normalization done with an anchor tag under a function so it will not be run on import

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 11, 2021

CLA Signed

The committers are authorized under a signed CLA.

@codecov
Copy link

codecov bot commented Mar 11, 2021

Codecov Report

Merging #2010 (4c38385) into main (e5fedbd) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 4c38385 differs from pull request most recent head b71cfb4. Consider uploading reports for the commit b71cfb4 to get more accurate results

@@           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           
Impacted Files Coverage Δ
...s/opentelemetry-instrumentation-fetch/src/fetch.ts 99.25% <100.00%> (+0.02%) ⬆️
packages/opentelemetry-web/src/utils.ts 94.80% <100.00%> (+0.13%) ⬆️

@@ -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'
Copy link
Member

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');
};

Copy link
Contributor Author

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?

Copy link
Member

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

Copy link
Member

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@obecny sounds good - thanks

Copy link
Contributor Author

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

@ryhinchey ryhinchey force-pushed the web-worker-ssr-support branch 2 times, most recently from cf60088 to 23cd68b Compare March 11, 2021 17:42
@ryhinchey ryhinchey changed the title Web worker ssr support Server side rendering support Mar 14, 2021
@ryhinchey
Copy link
Contributor Author

Is there anything else that needs to be done to land this? Thanks for the timely reviews!

@dyladan
Copy link
Member

dyladan commented Mar 23, 2021

Is there anything else that needs to be done to land this? Thanks for the timely reviews!

nope

@dyladan dyladan merged commit b50f837 into open-telemetry:main Mar 23, 2021
@dyladan dyladan added the enhancement New feature or request label Mar 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants