Skip to content

Commit

Permalink
Enable strict mode for transform-es2015-modules-commonjs
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@janicduplessis
janicduplessis authored and Facebook Github Bot 9 committed Mar 21, 2016
1 parent d033c45 commit 36893ec
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion babel-preset/configs/main.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ module.exports = {
'transform-es2015-computed-properties',
'transform-es2015-constants',
'transform-es2015-destructuring',
['transform-es2015-modules-commonjs', { strict: false, allowTopLevelThis: true }],
['transform-es2015-modules-commonjs', { allowTopLevelThis: true }],
'transform-es2015-parameters',
'transform-es2015-shorthand-properties',
'transform-es2015-spread',
Expand Down

8 comments on commit 36893ec

@keeth
Copy link

@keeth keeth commented on 36893ec Mar 23, 2016

Choose a reason for hiding this comment

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

the .babelrc override mentioned in the commit message doesn't seem to work - when I use it, strict mode is still enabled.

looks like the ability to override presets in Babel 6 is a work in progress.

@janicduplessis
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have you cleared the packager cache to make sure all the transforms are run again? (node ./node_modules/react-native/local-cli/cli.js start --reset-cache)

@keeth
Copy link

@keeth keeth commented on 36893ec Mar 23, 2016

Choose a reason for hiding this comment

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

Yep, it still happens.. I put together a minimal case.. just apply this patch over an empty 'react-native init' (RN 0.22) project. the error evaluating 'n.navigator', due to Firebase not being strict-mode compatible, happens even with a .babelrc that overrides the preset.

diff -rNu -x node_modules -x ios strictbug/.babelrc strictbug2/.babelrc
--- strictbug/.babelrc  1969-12-31 16:00:00.000000000 -0800
+++ strictbug2/.babelrc 2016-03-23 09:36:33.000000000 -0700
@@ -0,0 +1,6 @@
+{
+  "presets": ["react-native"],
+  "plugins": [
+    ["transform-es2015-modules-commonjs", { "strict": false, "allowTopLevelThis": true }]
+  ]
+}
diff -rNu -x node_modules -x ios strictbug/index.ios.js strictbug2/index.ios.js
--- strictbug/index.ios.js  2016-03-23 09:41:31.000000000 -0700
+++ strictbug2/index.ios.js 2016-03-23 09:35:27.000000000 -0700
@@ -11,6 +11,8 @@
   View
 } from 'react-native';

+import Firebase from 'firebase';
+
 class strictbug extends Component {
   render() {
     return (
diff -rNu -x node_modules -x ios strictbug/package.json strictbug2/package.json
--- strictbug/package.json  2016-03-23 09:41:43.000000000 -0700
+++ strictbug2/package.json 2016-03-23 09:46:05.000000000 -0700
@@ -6,6 +6,7 @@
     "start": "node node_modules/react-native/local-cli/cli.js start"
   },
   "dependencies": {
+    "firebase": "^2.4.1",
     "react": "^0.14.7",
     "react-native": "^0.22.2"
   }

@satya164
Copy link
Contributor

Choose a reason for hiding this comment

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

@keeth I think you can put firebase in Babel's ignore list.

@keeth
Copy link

@keeth keeth commented on 36893ec Mar 23, 2016

Choose a reason for hiding this comment

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

@satya164 yay, this fixed the issue of babel turning on strict mode for firebase.js, thank you!

echo node_modules/firebase/lib/firebase-web.js > .babelignore

I think the preset override issue still stands (that the {strict: false} override mentioned above appears to have no effect).

But putting firebase in .babelignore solves my original problem (I no longer need to keep babel-preset-react-native pinned at 1.5.2), so thanks!

@janicduplessis
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you put together a repro case and open a issue for that? I'll try to find out why it doesn't work anymore.

@satya164
Copy link
Contributor

Choose a reason for hiding this comment

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

@janicduplessis Applying the diff @keeth shared on a project generated by react-native init should do.

@keeth
Copy link

@keeth keeth commented on 36893ec Mar 23, 2016

Choose a reason for hiding this comment

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

Opened #6611

Thanks a lot for the ignore hint, it sounds like others are having this problem with Firebase (#6566).

Please sign in to comment.