-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
Make the packager work with babel strict mode transform #5422
Conversation
By analyzing the blame information on this pull request, we identified @vjeux, @spicyj and @davidaurelio to be potential reviewers. |
2261fba
to
9701013
Compare
@janicduplessis updated the pull request. |
@@ -25,11 +25,11 @@ | |||
require('regenerator/runtime'); | |||
|
|||
if (typeof GLOBAL === 'undefined') { | |||
GLOBAL = this; | |||
global.GLOBAL = this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we get rid of the GLOBAL
global variable and just use the node standard global
everywhere? Except for this file it is used in 1 or 2 other places in the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GLOBAL
is valid in Node so removing it will probably break some third party npm modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok let's leave it like this.
On a completely unrelated note that new @facebook-github-bot picture is amazing! |
This is awesome. I was planning to work on this, but seems now I don't have to :D Thanks for working on this. |
* The document must be shimmed before anything else that might define the | ||
* `ExecutionEnvironment` module (which checks for `document.createElement`). | ||
*/ | ||
/* eslint strict: 0 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why eslint strict: 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
global-strict
was replaced with strict
in eslint 1.0 so it was causing some lint warnings.
@janicduplessis, thank you for the effort. I’m generally fine with the changes. Let’s clarify the open questions, especially in polyfills/document.js |
@@ -239,4 +241,12 @@ function defineModuleCode(moduleName, code) { | |||
].join(''); | |||
} | |||
|
|||
function definePolyfillCode(code) { | |||
return [ | |||
'(function(global) {\n', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the line break here, otherwise line numbers in redboxes will be off by one for polyfills.
@janicduplessis updated the pull request. |
@davidaurelio Thanks for the review, I fixed the issues concerning the navigator polyfill and the other minor stuff. The issues left are the |
Awesome, thank you! Let me know if you need any help |
@davidaurelio Should I keep support for both |
@janicduplessis updated the pull request. |
Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/758031977665068/int_phab to review. |
@janicduplessis I’m sorry, but you have to rebase :-( |
25e951c
to
f12879b
Compare
@janicduplessis updated the pull request. |
At the moment we have to disable strict mode for the transform-es2015-modules-commonjs because strict mode leaks to the global scope and breaks the bridge. It was due to the way the polyfills were bundled in the package. To fix it, I wrapped the polyfill modules in an IIFE. Then when strict mode was enabled some polyfills were broken due to strict mode errors so that was fixed too. Also removed the IIFE from the polyfills that included one.
f12879b
to
34160e2
Compare
@janicduplessis updated the pull request. |
@davidaurelio Rebased on master and squashed the commits. |
I tried to land immediately after you posted yesterday, and right now, but there are still conflicts in Afaik there is no need to squash your commits, btw. They will be squashed by our system anyway, so don’t worry doing it. |
@davidaurelio Hmm weird, right now it says the branch has no conflicts. If you want to fix the conflicts manually what I did for these 2 files is simply take the version from master and then remove the extra closure scope that is not needed anymore. |
It seems like you still have the version before I rebased since that's the 2 conflicts I fixed, not sure how your system works exactly but maybe try reimporting it since I rebased it may have trouble updating my branch. |
ok, it’s supposed to update itself. Let’s try to ship from here. |
@facebook-github-bot shipit |
Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/758031977665068/int_phab to review. |
ok, the patch succeeded. Unless any internal tests fail this PR should get closed soon :-) |
🎉 |
👍 * 💯 |
d33b554
Summary: At the moment we have to disable strict mode for the transform-es2015-modules-commonjs because strict mode leaks to the global scope and breaks the bridge. It was due to the way the polyfills were bundled in the package. To fix it, I wrapped the polyfill modules in an IIFE. Then when strict mode was enabled some polyfills were broken due to strict mode errors so that was fixed too. Also removed the IIFE from the polyfills that included one. This diff doesn't enable the strict mode transform since some internal facebook modules depend on it not being enabled. When facebook#5214 lands we could make the default babel config shipped with OSS react-native use strict mode modules and facebook could just modify the babel config to disable it if needed. This will allow removing `"strict": false` from https://github.com/facebook/react-native/blob/master/packager/react-packager/.babelrc#L16 Fixes facebook#5316 Closes facebook#5422 Reviewed By: svcscm Differential Revision: D2846422 Pulled By: davidaurelio fb-gh-sync-id: a3e2f8909aa87dabab2b872c61b887e80220fb56
Summary: At the moment we have to disable strict mode for the transform-es2015-modules-commonjs because strict mode leaks to the global scope and breaks the bridge. It was due to the way the polyfills were bundled in the package. To fix it, I wrapped the polyfill modules in an IIFE. Then when strict mode was enabled some polyfills were broken due to strict mode errors so that was fixed too. Also removed the IIFE from the polyfills that included one. This diff doesn't enable the strict mode transform since some internal facebook modules depend on it not being enabled. When facebook#5214 lands we could make the default babel config shipped with OSS react-native use strict mode modules and facebook could just modify the babel config to disable it if needed. This will allow removing `"strict": false` from https://github.com/facebook/react-native/blob/master/packager/react-packager/.babelrc#L16 Fixes facebook#5316 Closes facebook#5422 Reviewed By: svcscm Differential Revision: D2846422 Pulled By: davidaurelio fb-gh-sync-id: a3e2f8909aa87dabab2b872c61b887e80220fb56
Summary:Since #5422 react-native works with strict mode modules but the transform was not updated since Facebook has some non strict mode compatible internal modules. Now that #5214 has landed and it is easy to change the babel config I think we should enable it by default to make es2015 modules spec compliant. Someone at Facebook will have to make the internal changes necessary to disable strict mode modules for their projects that use non strict mode compatible modules by including a .babelrc file with ``` json { "presets": [ "react-native" ], "plugins": [ ["transform-es2015-modules-commonjs", { "strict": false, "allowTopLevelThis": true }] ] } ``` before merging this. We might also want to mention this in the breaking change section for the next release. Closes #5796 Differential Revision: D3075802 fb-gh-sync-id: e807b67401107e1e944db38453e254025ce0a6c7 shipit-source-id: e807b67401107e1e944db38453e254025ce0a6c7
Summary:Since facebook#5422 react-native works with strict mode modules but the transform was not updated since Facebook has some non strict mode compatible internal modules. Now that facebook#5214 has landed and it is easy to change the babel config I think we should enable it by default to make es2015 modules spec compliant. Someone at Facebook will have to make the internal changes necessary to disable strict mode modules for their projects that use non strict mode compatible modules by including a .babelrc file with ``` json { "presets": [ "react-native" ], "plugins": [ ["transform-es2015-modules-commonjs", { "strict": false, "allowTopLevelThis": true }] ] } ``` before merging this. We might also want to mention this in the breaking change section for the next release. Closes facebook#5796 Differential Revision: D3075802 fb-gh-sync-id: e807b67401107e1e944db38453e254025ce0a6c7 shipit-source-id: e807b67401107e1e944db38453e254025ce0a6c7
Summary: At the moment we have to disable strict mode for the transform-es2015-modules-commonjs because strict mode leaks to the global scope and breaks the bridge. It was due to the way the polyfills were bundled in the package. To fix it, I wrapped the polyfill modules in an IIFE. Then when strict mode was enabled some polyfills were broken due to strict mode errors so that was fixed too. Also removed the IIFE from the polyfills that included one. This diff doesn't enable the strict mode transform since some internal facebook modules depend on it not being enabled. When #5214 lands we could make the default babel config shipped with OSS react-native use strict mode modules and facebook could just modify the babel config to disable it if needed. This will allow removing `"strict": false` from https://github.com/facebook/react-native/blob/master/packager/react-packager/.babelrc#L16 Fixes #5316 Closes facebook/react-native#5422 Reviewed By: svcscm Differential Revision: D2846422 Pulled By: davidaurelio fb-gh-sync-id: a3e2f8909aa87dabab2b872c61b887e80220fb56
At the moment we have to disable strict mode for the transform-es2015-modules-commonjs because strict mode leaks to the global scope and breaks the bridge. It was due to the way the polyfills were bundled in the package. To fix it, I wrapped the polyfill modules in an IIFE. Then when strict mode was enabled some polyfills were broken due to strict mode errors so that was fixed too. Also removed the IIFE from the polyfills that included one.
This diff doesn't enable the strict mode transform since some internal facebook modules depend on it not being enabled. When #5214 lands we could make the default babel config shipped with OSS react-native use strict mode modules and facebook could just modify the babel config to disable it if needed.
This will allow removing
"strict": false
from https://github.com/facebook/react-native/blob/master/packager/react-packager/.babelrc#L16Fixes #5316