Skip to content
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

Rely entirely on sourceType for module vs script differentiation. #7417

Merged
merged 1 commit into from
Feb 28, 2018

Conversation

loganfsmyth
Copy link
Member

@loganfsmyth loganfsmyth commented Feb 23, 2018

Q                       A
Fixed Issues? Fixes #6983
Patch: Bug Fix?
Major: Breaking Change? Yes (Vs early 7.x betas yes, from 6.x it depends on if we disable allowCommonJSExports by default (we didn't set this option, in this PR anyway, so things now behave like they did in Babel 6.x, but we have sourceType:unambiguous now as an alternative))
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT

Per #7416, we're in a weird place right now in Babel 7 because we've landed some logic that tries to be smart about whether something is a module or a script. The two points that affect this issue are:

  • If something is a ES6 module, we automatically add use strict and convert this to undefined.
  • In transform-runtime (and transform-async-to-generator when using module and method options), we try to insert either require or import based on the type of file.

So this puts us in a weird place.

In Babel 6, we just assumed it was always an ES6 module, which leads to breakage with Webpack because if transform-runtime inserts an import, but the user's file was CommonJS, Webpack will consider the whole file an ES6 module and the CommonJS stuff will break.

In Babel 7 we rectified that in the short term by basically saying it's an ES6 module if both:

  • It has sourceType: module
  • It has at least one import or export.

Unfortunately this means that if you want something to be an ES6 module and it doesn't have an import or export, it's actually impossible, hence #7416.

To resolve this, we've introduced sourceType: "unambiguous" that performs the above logic at the parser level, so instead of checking those 2 states during transformation, the parser produces either a script or module, which means that Babel itself can respect the sourceType as a source of truth.

With that work landed in the parser, this PR removes the logic from the transformation system. The downside here being that we don't want to default to sourceType: unambiguous because it isn't a formal specification and isn't backed by anything in particular. Instead we'd like to leave that up to the user to opt into.

This means that with this PR, Babel will revert to it's Babel 6 behavior as mentioned above for use strict and this rewriting.

I personally think that is fine. The main question I have is, should we try to make it more painful to treat a CommonJS module this way (auto-injecting use-strict and this->undefined) to encourage people to explicitly set sourceType based on what they actually want?

This could easily amount to us injecting code to throw a nice error saying "please set sourceType:script or sourceType:unambiguous in your Babel config". We already have an allowCommonJSExports option on the CommonJS transform that we could easily disable by default, though the message will need updating to be more helpful. If we don't, it'll just be like Babel 6 if someone runs transform-runtime on a CommonJS module, with modules:false for Webpack they'll get unhelpful error messages about module and exports being undeclared.

@@ -330,6 +330,7 @@ function run(task) {
const newOpts = merge(
{
filename: self.loc,
sourceType: "unambiguous",
Copy link
Member Author

Choose a reason for hiding this comment

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

It might be nice to ditch this and allow input.mjs and output.mjs files or something, then our tests will also assert that if a transform changed the sourceType, our input and output would have different extensions.

@loganfsmyth
Copy link
Member Author

The downside here being that we don't want to default to sourceType: unambiguous

If we should change the default to be sourceType:script is a whole discussion on its own, so I'd say if you want to comment on that, go to #6242

@babel-bot
Copy link
Collaborator

babel-bot commented Feb 23, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/6936/

@loganfsmyth loganfsmyth added the PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release label Feb 23, 2018
@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Feb 24, 2018

We already have an allowCommonJSExports option on the CommonJS transform that we could easily enable by default, though the message will need updating to be more helpful

Isn't it already enabled by default? I think that it shouldn't, since it is better to throw soon than making webpack throw later with a less descriptive error.

(Btw, is that option documented somewhere?)

@loganfsmyth
Copy link
Member Author

@nicolo-ribaudo Good catch, that's just a typo, definitely meant disable by default.

@loganfsmyth loganfsmyth merged commit ddd40bf into babel:master Feb 28, 2018
@loganfsmyth loganfsmyth deleted the sourcetype-reliance branch February 28, 2018 02:11
aminmarashi pushed a commit to aminmarashi/babel that referenced this pull request Mar 17, 2018
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Apr 19, 2018
Summary:
The `sourceType` option is a new mandatory option for Babel which determines whether to parse the file with the module or script goal. The value "disambiguous" determines this state by parsing.

See babel/babel#7417

Reviewed By: mjesun

Differential Revision: D7685610

fbshipit-source-id: 3958c5ad396592bb1d790e2df4ce315737421a2f
@Ailrun Ailrun mentioned this pull request May 25, 2018
2 tasks
emmatown pushed a commit to emotion-js/emotion that referenced this pull request May 25, 2018
**What**: Resolves #669 

**Why**: Current yarn.lock blocks dtslint to download new TS versions.

**How**: Update package.json and yarn.lock

**Checklist**:
<!-- add "N/A" to the end of each line that's irrelevant to your changes -->
<!-- to check an item, place an "x" in the box like so: "- [x] Documentation" -->
- [N/A] Documentation
- [x] Tests
- [x] Code complete

Snapshots of babel-plugin-emotion are changed when I update `@babel` things to `7.0.0-beta.41` or higher. I cannot figure out why... Maybe [this PR](babel/babel#7417) is related with the problem.
macdoum1 pushed a commit to macdoum1/react-native that referenced this pull request Jun 28, 2018
Summary:
The `sourceType` option is a new mandatory option for Babel which determines whether to parse the file with the module or script goal. The value "disambiguous" determines this state by parsing.

See babel/babel#7417

Reviewed By: mjesun

Differential Revision: D7685610

fbshipit-source-id: 3958c5ad396592bb1d790e2df4ce315737421a2f
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

babel-plugin-transform-runtime
3 participants