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

this change makes it so strict package managers installing duplicate … #1449

Conversation

void-mAlex
Copy link
Contributor

…versions of test-helpers across different addons use a shared global state instead of module state

@ef4
Copy link
Contributor

ef4 commented Jun 25, 2024

We discussed the polyfilling for globalThis and decide it's good. This is going to be back ported to majors where the polyfilling is needed. The implementation is inspired by the one in core-js, minus the parts that wouldn't possibly work inside an ES module.

@param {Object} it the global object to test
@returns {Boolean} it exists
*/
function check(it: any) {
Copy link
Sponsor Collaborator

Choose a reason for hiding this comment

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

why have this check?

why not:

const globalObject = 
  (typeof globalThis === 'object' && globalThis) ||
  (typeof window === 'object' && window) ||
  (typeof self === 'object' && self) ||
  (typeof global === 'object' && global);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

first because it's heavily tested code from core-js implementation
second it tests for actual global instead of accidental variables

void-mAlex and others added 3 commits July 19, 2024 17:46
…versions of test-helpers across different addons use a shared global state instead of module state
@NullVoxPopuli NullVoxPopuli force-pushed the make-set-get-context-use-an-actual-global-state branch from fc84403 to be76e64 Compare July 19, 2024 21:47
@NullVoxPopuli
Copy link
Sponsor Collaborator

This has failures right now, but it was previously passing, and the "engines" failures are due to floating deps that I can't control with yarn (I tried, and resolutions are brittle).

I'm going to merge this as is, then switch the repo to pnpm, add release-plan, and get a patch out

@NullVoxPopuli NullVoxPopuli merged commit caed007 into emberjs:master Jul 19, 2024
2 of 4 checks passed
@github-actions github-actions bot mentioned this pull request Jul 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants