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

fix: missing global in browser environments #1067

Merged
merged 1 commit into from
May 21, 2020

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented May 18, 2020

Which problem is this PR solving?

Short description of the changes

  • Add fallbacks for global accessing.

@legendecas
Copy link
Member Author

It turns out global shimming can not be turned off for assert depending on it...

@jufab
Copy link
Contributor

jufab commented May 18, 2020

I try this fix in my angular app and it works. 👍

@@ -38,7 +38,11 @@ type MyGlobals = Partial<{
[GLOBAL_TRACE_API_KEY]: Get<TracerProvider>;
}>;

export const _global = global as typeof global & MyGlobals;
export const _global = (typeof globalThis === 'object'
Copy link
Member

Choose a reason for hiding this comment

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

I think we should define global per platform and then use it here.
See example how it is done for example here
https://github.com/open-telemetry/opentelemetry-js/tree/b1255217a6f491756335fcb0945f4c8f205e0200/packages/opentelemetry-core/src/platform
You will need to define also a browser section in package.json.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, but this case is not identical to that one. Ideally, globalThis is preferred for its cross-platform design. However, globalThis only available after Node.js v12 and very new versions of browsers.

For those platform-dependent setups, I do believe they should go into this pattern. But for the global thing, I believe this is ok-ish (or globalThis would be referenced both in their platform/node/global.ts and platform/browser/global.ts).

Copy link
Member

Choose a reason for hiding this comment

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

You have already added a window which doesn't exist in node so this is wrong approach anyway and for such purposes the platform has been done.

Copy link
Member

Choose a reason for hiding this comment

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

and IE still doesn't support globalThis

Copy link
Member

Choose a reason for hiding this comment

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

As the person who caused this bug in the first place, think I'll defer to @obecny's web browser expertise in this regard. Generally, I don't think we should take advantage of features which are very new and only work on a small subset of very up-to-date installations.

@dyladan dyladan added the bug Something isn't working label May 18, 2020
@dyladan dyladan added this to High priority in Bug Tracker May 18, 2020
@codecov-commenter
Copy link

codecov-commenter commented May 19, 2020

Codecov Report

Merging #1067 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1067   +/-   ##
=======================================
  Coverage   94.97%   94.98%           
=======================================
  Files         235      238    +3     
  Lines        9081     9085    +4     
  Branches      821      822    +1     
=======================================
+ Hits         8625     8629    +4     
  Misses        456      456           
Impacted Files Coverage Δ
packages/opentelemetry-api/src/api/global-utils.ts 100.00% <100.00%> (ø)
packages/opentelemetry-api/src/platform/index.ts 100.00% <100.00%> (ø)
.../opentelemetry-api/src/platform/node/globalThis.ts 100.00% <100.00%> (ø)
...kages/opentelemetry-api/src/platform/node/index.ts 100.00% <100.00%> (ø)

Copy link
Member

@dyladan dyladan left a comment

Choose a reason for hiding this comment

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

This LGTM but I want to make sure we get feedback from @obecny

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

lgtm, thank you

@dyladan
Copy link
Member

dyladan commented May 19, 2020

@open-telemetry/javascript-approvers this is high-priority so please take a look. The API is currently broken in all web scenarios.

@mayurkale22
Copy link
Member

@dyladan @obecny Just wonder, why this is not captured by the build? Can we do something here to handle this kind of issues in the compile or build time?

@dyladan
Copy link
Member

dyladan commented May 19, 2020

I would have expected the browser tests to catch this. API tests must not be run in browser? I'll look into it.

@legendecas
Copy link
Member Author

As I mentioned above, webpack added shimming for global in testing so we can access global in the testing environment.

I was trying to disable webpack’s global shimming in testing, but the assert shimming is depending on it, it can not be turned off.

@legendecas legendecas requested a review from mwear as a code owner May 21, 2020 02:51
@dyladan
Copy link
Member

dyladan commented May 21, 2020

@legendecas please avoid force pushes where possible. It breaks several features of the github UI useful during code reviews.

@dyladan dyladan merged commit 661b8cf into open-telemetry:master May 21, 2020
Bug Tracker automation moved this from High priority to Closed May 21, 2020
@legendecas legendecas deleted the global branch May 26, 2020 02:22
dyladan pushed a commit to dyladan/opentelemetry-js that referenced this pull request Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Bug Tracker
  
Closed
Development

Successfully merging this pull request may close these issues.

Global is not defined in Angular App
7 participants