Enable strict mode for transform-es2015-modules-commonjs

[janicduplessis]

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

{
  "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.

[facebook-github-bot]

By analyzing the blame information on this pull request, we identified @skevy to be a potential reviewer.

[janicduplessis]

cc @davidaurelio @vjeux

[skevy]

cc @martinbigio

[facebook-github-bot]

@janicduplessis updated the pull request.

[facebook-github-bot]

@janicduplessis updated the pull request.

[satya164]

Would be awesome to get it merged.

[corbt]

Looks good to me, just need someone at Facebook to make the changes to their .babelrc's as necessary.

[bestander]

@martinbigio this is related to #6403 that I asked you to check.

[janicduplessis]

I retested this recently and it need #6255 to be merged to work properly or keep the allowTopLevelThis.

[facebook-github-bot]

@janicduplessis updated the pull request.

[bestander]

@janicduplessis does this need to be rebased?

[facebook-github-bot]

@janicduplessis updated the pull request.

[facebook-github-bot]

@janicduplessis updated the pull request.

[facebook-github-bot]

@janicduplessis updated the pull request.

[janicduplessis]

I removed the allowTopLevelThis since it's already addressed in #6255 and rebased on master. All tests should pass now.

[bestander]

@facebook-github-bot shipit

[facebook-github-bot]

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

[bestander]

@janicduplessis have you tested this with a manual babel transform?

Our apps internally use babel imperatively:

  var result = babel.transform(srcTxt, {
    retainLines: true,
    compact: true,
    comments: false,
    filename: filename,
    presets: ['react-native'],
    plugins: plugins,
    sourceFileName: filename,
    sourceMaps: options.generateSourceMaps,
  });

If I do

var var presetPlugins = require('babel-preset-react-native/plugins');
plugins.push([presetPlugins['babel-plugin-transform-es2015-modules-commonjs'], { strict: false, allowTopLevelThis: true }]);

before calling transform I still get "use strict" injected automatically

[janicduplessis]

No I haven't tested that. Did you clear the packager cache to make sure the transforms are all run again?

[janicduplessis]

Tested this and it worked:

var presetPlugins = require('babel-preset-react-native/plugins');
var babel = require('babel-core');
var plugins = [];
plugins.push([presetPlugins['babel-plugin-transform-es2015-modules-commonjs'], { strict: false, allowTopLevelThis: true }]);

var result = babel.transform('const a = 0;', {
  retainLines: true,
  compact: true,
  comments: false,
  presets: ['react-native'],
  plugins: plugins,
});

console.log(result.code);

It prints var a=0; when strict is false and "use strict";var a=0; when strict is true.

[bestander]

Thanks a lot, mate.
I'll dig deeper into packager unknowns.

[janicduplessis]

Good luck :)

[janicduplessis]

Ok, we should probably revert this. Not sure what changed in babel but changing a plugin option by defining it a second time doesn't work anymore. Since we transform all the code in node_modules by default there are a lot of issues with libraries that don't support strict mode. Right now the only way to disable it is to not use babel-preset-react-native and define all the transforms manually so it is not very convenient.

Let's wait until there is a supported way to pass options to presets in babel so disabling strict mode is easy. Also I know there was already some discussion about that but shipping react-native and libraries already compiled would help a lot here.

cc @bestander

[bestander]

Sounds good to me.
Will you send a pr?

On Sunday, 27 March 2016, Janic Duplessis notifications@github.com wrote:

Ok, we should probably revert this. Not sure what changed in babel but
changing a plugin option by defining it a second time doesn't work anymore.
Since we transform all the code in node_modules by default there are a lot
of issues with libraries that don't support strict mode. Right now the only
way to disable it is to not use babel-preset-react-native and define all
the transforms manually so it is not very convenient.

Let's wait until there is a supported way to pass options to presets in
babel so disabling strict mode is easy.

—
You are receiving this because you were assigned.
Reply to this email directly or view it on GitHub
#5796 (comment)

[satya164]

There's always a risk of breaking third party code since we're transpiling things under node_modules. What would be great is if we could precompile RN before distributing (doesn't need to be a bundle, just transpile with babel, and rename source files to *.flow). Then we could avoid transpiling node_modules altogether and anyone can use any preset they want.

[bestander]

Makes sense actually.
So just make it commonjs es5 code?

On Sunday, 27 March 2016, Satyajit Sahoo notifications@github.com wrote:

There's always a risk of breaking third party code since we're transpiling
things under node_modules. What would be great is if we could precompile
RN before distributing (doesn't need to be a bundle, just transpile with
babel, and rename source files to *.flow). Then we could avoid
transpiling node_modules altogether and anyone can use any preset they want.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5796 (comment)

[satya164]

@bestander Yeah. And keep the original files as *.flow files, so flow works for consumers.

[bestander]

I know David Aurelio wanted to get rid of haste code with transpiling
before npm publish.
You propose the next logical step.

A discussion in Core Contributors group maybe?

On Sunday, 27 March 2016, Satyajit Sahoo notifications@github.com wrote:

@bestander https://github.com/bestander Yeah. And keep the original
files as .flow files, so flow works for consumers.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5796 (comment)

[satya164]

@bestander Yeah, will post in the group. :)

[janicduplessis]

@bestander #6686