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

Remove window.Ember global #19678

Merged
merged 1 commit into from
Aug 17, 2021
Merged

Conversation

mixonic
Copy link
Member

@mixonic mixonic commented Jul 23, 2021

Per #19617

@mixonic mixonic requested review from rwjblue and chancancode and removed request for rwjblue July 23, 2021 13:10
@mixonic mixonic marked this pull request as ready for review July 23, 2021 13:10
@mixonic mixonic requested a review from rwjblue July 23, 2021 13:10
@@ -43,7 +43,7 @@ function buildSandboxContext(precompile) {
URL,

// Convince jQuery not to assume it's in a browser
module: { exports: {} },
module: { exports: {}, require() {} },
Copy link
Member Author

Choose a reason for hiding this comment

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

This is maybe the most suspicious change in here. As these tests are simply executing scripts in a context, they don't directly use any module system (CommonJS or ES). Adding require() {} to the exports fools Ember into thinking this is a CommonJS env (which is effectively what this line was doing anyway) and thus Ember sets .exports to be Ember. This hack only works for one this-which-exports-via-CommonJS in a context.

I'm not familiar enough with how this code runs in Fastboot to know if this is a meaningful test. cc @rwjblue

Copy link
Member Author

Choose a reason for hiding this comment

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

@rwjblue suggested this should land, and that if Fastboot as a follow up we can address it during the beta.

@mixonic mixonic merged commit 8c5b777 into emberjs:master Aug 17, 2021
@mixonic mixonic deleted the mixonic/drop-global branch August 17, 2021 01:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants