Make the packager work with babel strict mode transform by janicduplessis · Pull Request #5422 · react/react-native

janicduplessis · 2016-01-20T02:05:30Z

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

facebook-github-bot · 2016-01-20T02:06:37Z

By analyzing the blame information on this pull request, we identified @vjeux, @spicyj and @davidaurelio to be potential reviewers.

facebook-github-bot · 2016-01-20T02:08:21Z

@janicduplessis updated the pull request.

janicduplessis · 2016-01-20T02:09:46Z

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.

GLOBAL is valid in Node so removing it will probably break some third party npm modules.

Ok let's leave it like this.

janicduplessis · 2016-01-20T03:06:23Z

On a completely unrelated note that new @facebook-github-bot picture is amazing!

satya164 · 2016-01-20T04:40:14Z

This is awesome. I was planning to work on this, but seems now I don't have to :D

Thanks for working on this.

satya164 · 2016-01-20T04:41:48Z

Why eslint strict: 0?

global-strict was replaced with strict in eslint 1.0 so it was causing some lint warnings.

davidaurelio · 2016-01-20T09:40:17Z

@janicduplessis, thank you for the effort. I’m generally fine with the changes. Let’s clarify the open questions, especially in polyfills/document.js

davidaurelio · 2016-01-20T12:01:07Z

Please remove the line break here, otherwise line numbers in redboxes will be off by one for polyfills.

facebook-github-bot · 2016-01-20T13:30:16Z

@janicduplessis updated the pull request.

janicduplessis · 2016-01-20T14:04:33Z

@davidaurelio Thanks for the review, I fixed the issues concerning the navigator polyfill and the other minor stuff. The issues left are the @polyfill thing and WPO. I tested WPO on master vs my branch and it is effectively broken (__DEV__ code is not being removed). I'll check what has to change to make it work.

davidaurelio · 2016-01-20T14:07:29Z

Awesome, thank you! Let me know if you need any help

janicduplessis · 2016-01-20T14:56:12Z

@davidaurelio Should I keep support for both __DEV__ and global.__DEV__?

facebook-github-bot · 2016-01-20T16:06:20Z

@janicduplessis updated the pull request.

facebook-github-bot · 2016-01-20T21:14:29Z

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.

davidaurelio · 2016-01-20T21:27:33Z

@janicduplessis I’m sorry, but you have to rebase :-(

facebook-github-bot · 2016-01-20T22:03:19Z

@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.

facebook-github-bot · 2016-01-20T22:10:18Z

@janicduplessis updated the pull request.

janicduplessis · 2016-01-20T22:14:22Z

@davidaurelio Rebased on master and squashed the commits.

davidaurelio · 2016-01-21T11:58:47Z

I tried to land immediately after you posted yesterday, and right now, but there are still conflicts in packager/react-packager/src/Resolver/polyfills/babelHelpers.js and packager/react-packager/src/Resolver/polyfills/error-guard.js.

Afaik there is no need to squash your commits, btw. They will be squashed by our system anyway, so don’t worry doing it.

janicduplessis · 2016-01-21T13:38:57Z

@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.

janicduplessis · 2016-01-21T13:58:06Z

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.

davidaurelio · 2016-01-21T14:15:52Z

ok, it’s supposed to update itself. Let’s try to ship from here.

davidaurelio · 2016-01-21T14:16:02Z

@facebook-github-bot shipit

facebook-github-bot · 2016-01-21T14:16:40Z

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.

davidaurelio · 2016-01-21T14:32:20Z

ok, the patch succeeded. Unless any internal tests fail this PR should get closed soon :-)

satya164 · 2016-01-21T14:52:28Z

🎉

corbt · 2016-01-21T15:04:01Z

👍 * 💯

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 react#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#L16Fixesreact#5316Closesreact#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: 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 Closesreact/react-native#5422 Reviewed By: svcscm Differential Revision: D2846422 Pulled By: davidaurelio fb-gh-sync-id: a3e2f8909aa87dabab2b872c61b887e80220fb56

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jan 20, 2016

janicduplessis force-pushed the strict-mode branch from 2261fba to 9701013Compare January 20, 2016 02:07

janicduplessis reviewed Jan 20, 2016
View reviewed changes

satya164 reviewed Jan 20, 2016
View reviewed changes

davidaurelio reviewed Jan 20, 2016
View reviewed changes

davidaurelio self-assigned this Jan 20, 2016

davidaurelio added GH Review: needs-revision and removed GH Review: review-needed labels Jan 20, 2016

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Jan 20, 2016

davidaurelio added GH Review: needs-revision and removed GH Review: review-needed labels Jan 20, 2016

janicduplessis force-pushed the strict-mode branch from 25e951c to f12879bCompare January 20, 2016 22:02

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Jan 20, 2016

janicduplessis force-pushed the strict-mode branch from f12879b to 34160e2Compare January 20, 2016 22:10

ghost closed this in d33b554Jan 21, 2016

janicduplessis deleted the strict-mode branch February 7, 2016 06:53

janicduplessis mentioned this pull request Feb 7, 2016
Enable strict mode for transform-es2015-modules-commonjs #5796
Closed

janicduplessis mentioned this pull request Mar 10, 2016
Removed eslint rule that checks modules #6403
Closed

This pull request was closed.

Uh oh!

Conversation

janicduplessis commented Jan 20, 2016

Uh oh!

facebook-github-bot commented Jan 20, 2016

Uh oh!

facebook-github-bot commented Jan 20, 2016

Uh oh!

janicduplessisJan 20, 2016

Choose a reason for hiding this comment

Uh oh!

sebmckJan 20, 2016

Choose a reason for hiding this comment

Uh oh!

janicduplessisJan 20, 2016

Choose a reason for hiding this comment

Uh oh!

janicduplessis commented Jan 20, 2016

Uh oh!

satya164 commented Jan 20, 2016

Uh oh!

satya164Jan 20, 2016

Choose a reason for hiding this comment

Uh oh!

janicduplessisJan 20, 2016

Choose a reason for hiding this comment

Uh oh!

davidaurelio commented Jan 20, 2016

Uh oh!

davidaurelioJan 20, 2016

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Jan 20, 2016

Uh oh!

janicduplessis commented Jan 20, 2016

Uh oh!

davidaurelio commented Jan 20, 2016

Uh oh!

janicduplessis commented Jan 20, 2016

Uh oh!

facebook-github-bot commented Jan 20, 2016

Uh oh!

facebook-github-bot commented Jan 20, 2016

Uh oh!

davidaurelio commented Jan 20, 2016

Uh oh!

facebook-github-bot commented Jan 20, 2016

Uh oh!

facebook-github-bot commented Jan 20, 2016

Uh oh!

janicduplessis commented Jan 20, 2016

Uh oh!

davidaurelio commented Jan 21, 2016

Uh oh!

janicduplessis commented Jan 21, 2016

Uh oh!

janicduplessis commented Jan 21, 2016

Uh oh!

davidaurelio commented Jan 21, 2016

Uh oh!

davidaurelio commented Jan 21, 2016

Uh oh!

facebook-github-bot commented Jan 21, 2016

Uh oh!

davidaurelio commented Jan 21, 2016

Uh oh!

satya164 commented Jan 21, 2016

Uh oh!

corbt commented Jan 21, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants