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

[BUGFIX beta] Add back mainContext to loader #14859 #14860

Merged
merged 1 commit into from
Jan 22, 2017

Conversation

zoltan-nz
Copy link
Contributor

@zoltan-nz zoltan-nz commented Jan 21, 2017

Fixing #14859

After the ESLint warning cleanup the mainContext is undefined and brake the ember-template-compiler.js compilation in grunt environment.

This PR removes the rest of the mainContext reference from the global.js.

Previous conversation about this issue:
e24e403#diff-5b2ea89606fd2097e9e17d66ae54ed51L2

Opened issue in grunt-ember-templates repo: dgeb/grunt-ember-templates#94

cc @Turbo87 @andyhot

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! We need to bring back the line that was removed in the loader. If you can update then I'm happy to land this...

@@ -14,5 +14,4 @@ function checkElementIdShadowing(value) {
export default checkGlobal(checkElementIdShadowing(typeof global === 'object' && global)) ||
checkGlobal(typeof self === 'object' && self) ||
checkGlobal(typeof window === 'object' && window) ||
mainContext || // set before strict mode in Ember loader/wrapper
Copy link
Member

Choose a reason for hiding this comment

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

Can you bring this back, and add it back to the correct location in loader.js? The ESLint cleanup PR should not have removed it (and I missed that it did when I reviewed).

Copy link
Member

Choose a reason for hiding this comment

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

Sorry about that. Can we add a comment to it this time describing what the unused variable does?

Copy link

Choose a reason for hiding this comment

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

And also add a // eslint-disable-next-line no-unused-vars line before the mainContext definition in packages/loader/lib/index.js (even though no-unused-vars is globally off for now)

Still, i'm a bit puzzled by the global.js code... If mainContext is defined nowhere, the code will still fail. And if it is, the next line new Function('return this')() will never run.

Would it make sense to replace mainContext with (typeof mainContext === 'object' && mainContext) ?

@zoltan-nz
Copy link
Contributor Author

@rwjblue @Turbo87 mainContext added back to the loader with a tiny comment.

@zoltan-nz zoltan-nz changed the title [BUGFIX beta] Remove mainContext from global.js fix #14859 [BUGFIX beta] Add back mainContext to loader #14859 Jan 22, 2017
@rwjblue rwjblue merged commit 70a94bb into emberjs:master Jan 22, 2017
@rwjblue
Copy link
Member

rwjblue commented Jan 22, 2017

Thank you!

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.

4 participants