Remove the need for allowTopLevelThis in transform-es2015-modules-commonjs #6255

janicduplessis · 2016-03-03T00:36:29Z

This make the transform behave closer to the standard for modules.

This removes the few places that a top level this was used to refer to the global space. It also clean up the usage of GLOBAL to use global instead as this is what is used everywhere else in the code base. We still define GLOBAL for compatibility with other modules.

Test plan
Clear the packager cache to make sure the transforms run again. (node ./local-cli/cli.js start --reset-cache).
Run the Movies example (UIExplorer is broken atm) and make sure there are no errors.

facebook-github-bot · 2016-03-03T00:37:24Z

By analyzing the blame information on this pull request, we identified @sahrens, @janicduplessis and @frantic to be potential reviewers.

eslint-bot · 2016-03-03T02:02:57Z

Libraries/Utilities/ErrorUtils.js

@@ -24,4 +21,4 @@ var GLOBAL = this;
 * that use it aren't just using a global variable, so simply export the global
 * variable here. ErrorUtils is originally defined in a file named error-guard.js.
 */
-module.exports = GLOBAL.ErrorUtils;


strict: Use the global form of "use strict".

facebook-github-bot · 2016-03-03T02:19:17Z

@janicduplessis updated the pull request.

janicduplessis · 2016-03-06T23:30:16Z

Cc @davidaurelio

ide · 2016-03-07T21:44:38Z

LGTM

ide · 2016-03-10T20:50:41Z

Tests are green and this looks safe to me. Mainly it's a question of whether some FB internal code is relying on some subtle behavior that may have changed -- shipping this to send it to the internal test runner.

ide · 2016-03-10T20:50:46Z

@facebook-github-bot shipit

facebook-github-bot · 2016-03-10T20:51:30Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

ide · 2016-03-10T20:51:58Z

Libraries/Utilities/ErrorUtils.js

-/* eslint-disable consistent-this, global-strict */
-
-var GLOBAL = this;
+/* eslint-disable strict */


Also we probably can write "use strict" now... can do that in a future diff.

We were actually discussing enabling strict mode in modules and maybe remove 'use strict' added manually everywhere so we can leave it as is for now.

janicduplessis · 2016-03-10T21:01:21Z

👍

ide · 2016-03-11T00:06:04Z

@davidaurelio @martinbigio I suspect this caused some internal tests to fail -- if this is the case, is it possible for FB to use a custom Babel configuration that extends the RN preset but sets allowTopLevelThis to true?

facebook-github-bot · 2016-03-15T17:16:23Z

@janicduplessis updated the pull request.

mkonicek · 2016-03-16T15:48:09Z

@davidaurelio Is this going to break anything?

satya164 · 2016-03-21T21:40:49Z

With this change, we can finally use 'es2015' preset instead of writing individual plugins, right?

Thanks a lot for working on this. Moving the preset closer towards standard setups is really great.

satya164 · 2016-04-04T16:21:57Z

Any updates on this?

facebook-github-bot · 2016-04-05T03:13:42Z

@janicduplessis updated the pull request.

janicduplessis · 2016-04-05T03:16:18Z

What I think we should do for now is keep the allowTopLevelThis in the preset but still make these changes that allow RN to work without it. The main issue right now is that we transform every library in node_modules with the babel preset so it can cause issues that are hard to fix. Until we can configure the babel preset it's better to keep the transforms as loose a possible.

janicduplessis · 2016-04-05T03:18:01Z

I put back the allowTopLevelThis in the babel preset so it should be able to get merged without breaking internal fb tests.

ide · 2016-04-05T04:27:36Z

@facebook-github-bot shipit

davidaurelio · 2016-04-06T14:08:16Z

@facebook-github-bot shipit

facebook-github-bot · 2016-04-06T14:10:23Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

mkonicek · 2016-04-07T13:58:02Z

Flaky CI maybe?

@facebook-github-bot shipit

facebook-github-bot · 2016-04-07T13:59:47Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

satya164 · 2016-04-07T14:11:47Z

Seems the tests are not passing. Some error with code analysis bot.

facebook-github-bot · 2016-04-10T18:14:14Z

@janicduplessis updated the pull request.

satya164 · 2016-04-10T20:59:10Z

Seems Flow check is failing in the CI

janicduplessis · 2016-04-10T21:15:48Z

No idea what causes that. Flow is scanning random buck json files.

cc @bestander @mkonicek

bestander · 2016-04-11T14:52:03Z

@janicduplessis, no worries, it is some old Circle cache that had buck installed in the project file tree.
I set it to rebuild without caches, should be fine

mkonicek · 2016-04-22T14:27:33Z

@facebook-github-bot shipit

facebook-github-bot · 2016-04-22T14:28:53Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

janicduplessis · 2016-04-26T08:13:43Z

@mkonicek Any idea what caused the internal test failures here?

…monjs This removes the few places that a top level this was used to refer to the global space. It also clean up the usage of `GLOBAL` to use `global` instead as this is what is used everywhere else in the code base. We still define `GLOBAL` for compatibility with other modules.

ghost · 2016-04-29T06:39:13Z

@janicduplessis updated the pull request.

ghost · 2016-04-29T06:42:21Z

@janicduplessis updated the pull request.

mkonicek · 2016-04-29T10:09:26Z

I can't tell what's wrong with the tests actually, a generic Buck failure. Merging again.

davidaurelio · 2016-04-29T21:32:03Z

@janicduplessis I’d really like to have this. Would you mind rebasing again? I’ll try to get this in ;-(

janicduplessis · 2016-04-29T21:35:17Z

Rebased it yesterday, it should be fine now.

davidaurelio · 2016-04-29T22:15:14Z

@facebook-github-bot shipit

ghost · 2016-04-29T22:16:51Z

Thanks for importing. If you are an FB employee go to Phabricator to review.

…monjs Summary: This make the transform behave closer to the standard for modules. This removes the few places that a top level this was used to refer to the global space. It also clean up the usage of `GLOBAL` to use `global` instead as this is what is used everywhere else in the code base. We still define `GLOBAL` for compatibility with other modules. **Test plan** Clear the packager cache to make sure the transforms run again. (node ./local-cli/cli.js start --reset-cache). Run the Movies example (UIExplorer is broken atm) and make sure there are no errors. Closes facebook#6255 Differential Revision: D3037227 Pulled By: mkonicek fb-gh-sync-id: bcf1350ae7a6e92c77d3a87fc9d6e42eb93cb9b9 fbshipit-source-id: bcf1350ae7a6e92c77d3a87fc9d6e42eb93cb9b9

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 Mar 3, 2016

eslint-bot reviewed Mar 3, 2016
View reviewed changes

janicduplessis force-pushed the top-level-this branch from c60e319 to 7c749d9 Compare March 3, 2016 02:18

janicduplessis mentioned this pull request Mar 10, 2016

Enable strict mode for transform-es2015-modules-commonjs #5796

Closed

facebook-github-bot added GH Review: accepted and removed GH Review: review-needed labels Mar 10, 2016

ide reviewed Mar 10, 2016
View reviewed changes

mkonicek removed the GH Review: accepted label Mar 16, 2016

mkonicek assigned davidaurelio Mar 16, 2016

facebook-github-bot added the GH Review: review-needed label Apr 5, 2016

facebook-github-bot removed the GH Review: review-needed label Apr 6, 2016

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Apr 22, 2016

facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Apr 23, 2016

janicduplessis force-pushed the top-level-this branch from 5eef7a0 to 8f866c4 Compare April 29, 2016 06:38

Keep allowTopLevelThis in the babel preset

df69741

janicduplessis force-pushed the top-level-this branch from 8f866c4 to df69741 Compare April 29, 2016 06:41

ghost added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Apr 29, 2016

ghost closed this in 169cfb5 Apr 29, 2016

This pull request was closed.

Remove the need for allowTopLevelThis in transform-es2015-modules-commonjs #6255

Remove the need for allowTopLevelThis in transform-es2015-modules-commonjs #6255

Uh oh!

Conversation

janicduplessis commented Mar 3, 2016

Uh oh!

facebook-github-bot commented Mar 3, 2016

Uh oh!

eslint-bot Mar 3, 2016

Choose a reason for hiding this comment

Uh oh!

facebook-github-bot commented Mar 3, 2016

Uh oh!

janicduplessis commented Mar 6, 2016

Uh oh!

ide commented Mar 7, 2016

Uh oh!

ide commented Mar 10, 2016

Uh oh!

ide commented Mar 10, 2016

Uh oh!

facebook-github-bot commented Mar 10, 2016

Uh oh!

ide Mar 10, 2016

Choose a reason for hiding this comment

Uh oh!

janicduplessis Mar 10, 2016

Choose a reason for hiding this comment

Uh oh!

janicduplessis commented Mar 10, 2016

Uh oh!

ide commented Mar 11, 2016

Uh oh!

facebook-github-bot commented Mar 15, 2016

Uh oh!

mkonicek commented Mar 16, 2016

Uh oh!

satya164 commented Mar 21, 2016

Uh oh!

satya164 commented Apr 4, 2016

Uh oh!

facebook-github-bot commented Apr 5, 2016

Uh oh!

janicduplessis commented Apr 5, 2016

Uh oh!

janicduplessis commented Apr 5, 2016

Uh oh!

ide commented Apr 5, 2016

Uh oh!

davidaurelio commented Apr 6, 2016

Uh oh!

facebook-github-bot commented Apr 6, 2016

Uh oh!

mkonicek commented Apr 7, 2016

Uh oh!

facebook-github-bot commented Apr 7, 2016

Uh oh!

satya164 commented Apr 7, 2016

Uh oh!

facebook-github-bot commented Apr 10, 2016

Uh oh!

satya164 commented Apr 10, 2016

Uh oh!

janicduplessis commented Apr 10, 2016

Uh oh!

bestander commented Apr 11, 2016

Uh oh!

mkonicek commented Apr 22, 2016

Uh oh!

facebook-github-bot commented Apr 22, 2016

Uh oh!

janicduplessis commented Apr 26, 2016

Uh oh!

ghost commented Apr 29, 2016

Uh oh!

ghost commented Apr 29, 2016

Uh oh!

mkonicek commented Apr 29, 2016

Uh oh!

davidaurelio commented Apr 29, 2016