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

[8.x] Gain consensus on path forward for "sourceType" for parsing modules vs scripts #6242

Open
loganfsmyth opened this Issue Sep 13, 2017 · 20 comments

Comments

Projects
None yet
6 participants
@loganfsmyth
Copy link
Member

loganfsmyth commented Sep 13, 2017

Rather than have this on Slack, I thought it would be useful to have this as an issue so we can refer to it in the future. So, the question:

Should we switch from sourceType: "module" for .js files, to sourceType: "script".

What this means in practice is:

  • Users using ES6 module syntax will have to rename their files to .mjs or put sourceType: "module" in their .babelrc or whatever Babel config they have.
  • Users who were relying on Babel automatically adding use strict to their CommonJS files will have to add transform-strict-mode to their Babel configs.

Because Babel has parsed as module by default for a long time, it has logic that essentially says "if there are no imports/exports in this module, it's actually a script". This adds a lot of complexity to Babel's module processing because this logic is not standard in any way, and it means that there is no central way for code to ask "is this a module".

The reason "is this a module" is such an important question is that because of this, currently it is valid to have both

  1. ES6 import statements and require() usage
  2. ES6 export statements and module.exports/exports.foo usage

Webpack now disallows the #2 case, while allowing #1. Ideally Babel should also adopt that behavior, but where it's not too bad for Webpack, Babel's iterative transformational nature makes it a pain to constantly not know what type of sourceType the file should be considered as.

My proposal is the following:

  • IN PR(#7501): Change sourceType to default to script for .js, and force to module for .mjs
  • DONE(via flag on CommonJS transform): Disallow use of CommonJS's module.exports/exports when sourceType: module (potentially with a flag to enable this again to ease transition)
  • DONE: Update the parts of Babel that inject import statements to be smart enough to inject both import and require based on the .sourceType

To ease the transition, I'd propose a few more possibilities.

  • DONE: Land sourceType: "unambiguous" parsing so that users with codebases that aggressively mix ES6 modules and CommonJS modules don't pull their hair out. This should essentially make parsing act like it did before, minus the use strict injection. Support for this has landed in #6789.
  • IN PR(#7502): Fix Ability to output mjs extension by having babel-cli output files with a .mjs extension if they have sourceType: 'module'
  • DONE: Support Configuration based on file path so that if people don't want to use sourceType: 'unambiguous', they can at least explicitly opt specific files into a sourceType

What do people think?

@Jessidhia

This comment has been minimized.

Copy link
Member

Jessidhia commented Sep 15, 2017

One possible difficulty is what to do with .ts and .tsx files. The babel typescript transform is written with the idea that --isolatedModules is used so they are always modules but if the file only uses import BindingIdentifier = require('request') and export = Expression, it is still a commonjs script. 🤔

In TypeScript with --isolatedModules, though, having no imports or exports at all in a file is a syntax error, so it avoids the hardest case of unambiguous parsing.

@loganfsmyth

This comment has been minimized.

Copy link
Member Author

loganfsmyth commented Sep 19, 2017

Another way to look at this is that right now, without modifying the code of the file itself with things like export {};, there's literally no way to tell Babel to treat a given snippet of code as a module. As a hack it can check the extension, but what if there's no filename given? If we do inject export {};, that'll show up in an output sourcemap.

Being confident of when something is a module is important because things need to know this stuff to know what the properties of the file are. Can a transform inject a with statement? No idea, because you can't tell if the original file is a script or a module. Want to know if code is running in strict mode? You can't.

Even in our own unit tests, which are .js files currently, do we transform this to undefined? We do now because the module transform runs on every file and never checks sourceType at all, which means Babel breaks people's code if it gets run on things in node_modules. To fix that it would only run when a file is a module, but now sourceType isn't enough, so it also needs to check the file extension.

This is all a huge pain for users that we've essentially glossed over because most people don't run into it, but it's only going to happen more and more as stuff in node_modules starts shipping with more ES6 inside of it. We're going to need to make it easier for people to compile node_modules, however unfortunate that may be for performance, and right now that breaks code in wild.

We can also discuss defaulting to "unambiguous" parsing, but I think it's important that we default to standards-compliant behavior, even if we provide more flexibility as an opt-in.

@Jessidhia

This comment has been minimized.

Copy link
Member

Jessidhia commented Sep 20, 2017

The difficulty with supporting that for node_modules compilation is that the setting that is appropriate for the user's project is not necessarily the correct setting for any given node_modules module, and there may be multiple different node_modules entries each requiring a different setting.

This problem should also increase as node modules start shipping .mjs files. Probably the easiest solution is to use file extension (always module for .mjs), or fallback to sourceType if the extension is .js or otherwise unavailable. .ts / .tsx might need special handling as I mentioned earlier -- the common case is definitely module, but it's possible to be script if the file has import Identifier = or export =.

A possible way to handle TypeScript files, though, is to just always treat them as modules. If TypeScript's module option is set to es2015 or esnext, it is a syntax error to write import Identifier = or export = (outside of .d.ts files) anyway, so we could just make those always syntax errors too (unless there is a need to be permissive for static analysis?). I suspect that projects that depend on babel's typescript support and depend on script behavior are nonexistent.

@ljharb

This comment has been minimized.

Copy link

ljharb commented Sep 20, 2017

Just dropping in to say that in no way should "unambiguous" be the default - it's nonstandard, and the only defaults should ever be "the standard". Adding it as an option is great! But it should only be explicitly opt-in.

I think that the proper thing is to do what node is planning, which is ".mjs is Module, .js is Script" by default, and if users want to override this by extension they should have the ability to do that. That means that .ts could be defined to be Module, or .js "unambiguous", or whatever - but the default would be "what node does".

@bmeck

This comment has been minimized.

Copy link
Contributor

bmeck commented Oct 4, 2017

@loganfsmyth

As a hack it can check the extension, but what if there's no filename given?

Are there clear examples of this right now? Also things are starting to look like you can declare the media type for source text and Node is moving it's interpretation of .js/CJS to be under text/vnd.node.js (please don't use this til registration is done).

@ljharb

This comment has been minimized.

Copy link

ljharb commented Oct 4, 2017

How would a MIME type be relevant to non-browsers tho? Babel transpiles for server apps too.

@bmeck

This comment has been minimized.

Copy link
Contributor

bmeck commented Oct 4, 2017

@ljharb various useful APIs, like Blob and URL.createObjectURL use MIME

@ljharb

This comment has been minimized.

Copy link

ljharb commented Oct 4, 2017

I didn't realize those worked in node, but I'm still not understanding how a mime type is relevant to how Babel parses things :-/

@bmeck

This comment has been minimized.

Copy link
Contributor

bmeck commented Oct 4, 2017

@ljharb because if they have a "sourceMode" having it be MIME based would be great. https://github.com/bmeck/I-D/blob/master/javascript-mjs/draft.md#textjavascript updated internet draft also added a goal parameter per review in bmeck/I-D#1

@ljharb

This comment has been minimized.

Copy link

ljharb commented Oct 4, 2017

Right, but what besides the extension informs the mime type?

@bmeck

This comment has been minimized.

Copy link
Contributor

bmeck commented Oct 4, 2017

@ljharb for file backed source text extension mostly informs the type. However .js remains ambiguous if you target browsers using Script vs browsers using CJS. No one targets using Script to my knowledge so it isn't really an issue. for non-file backed source texts (things that are using mechanics like WebPack's emitFile APIs the type is declared via the loader).

@ljharb

This comment has been minimized.

Copy link

ljharb commented Oct 4, 2017

Right, but babel doesn't target browsers; that's a bundler's job.

@bmeck

This comment has been minimized.

Copy link
Contributor

bmeck commented Oct 4, 2017

@ljharb it should still use the MIME under the hood when there are not files instead of a random enum.

@ljharb

This comment has been minimized.

Copy link

ljharb commented Oct 4, 2017

Without an extension, where would it get the MIME from? (Sorry if I'm being dense here; I just don't see how mime types ever matter before the point of sending a response to a browser)

@bmeck

This comment has been minimized.

Copy link
Contributor

bmeck commented Oct 4, 2017

@ljharb a variety of potential places, mostly from generated source that is not on disk.

@benjamn

This comment has been minimized.

Copy link
Contributor

benjamn commented Nov 22, 2017

  • Disallow use of CommonJS's module.exports/exports when sourceType: module (potentially with a flag to enable this again to ease transition)

This seems like a needless restriction for Babel to enforce. The environment that I choose to provide to my Babel-compiled code (including require, exports, and module, or anything else) should not be any concern of Babel's.

@loganfsmyth

This comment has been minimized.

Copy link
Member Author

loganfsmyth commented Nov 23, 2017

But that means we're not actually enforcing the proper ES6 semantics. If I write

export const Foo = 4;

window.module = {exports: 5};
console.log(module.exports.Foo);

in an ES6 module, that's reading module from the global, per the spec. We can't enforce that because we can't rename the binding that the CommonJS wrapper enforces, but we can at least give users a clear error.

I believe Webpack already mixing ES6 and CommonJS exports because it uses a different wrapper for modules that it detects as ES6, so the module and exports bindings are not available and actually properly refer to the global.

We can always have a flag to opt out of it, but we're doing our best to move toward our default being spec compliance.

@loganfsmyth

This comment has been minimized.

Copy link
Member Author

loganfsmyth commented Dec 17, 2017

@loganfsmyth

This comment has been minimized.

Copy link
Member Author

loganfsmyth commented Mar 5, 2018

Alright, I've posted #7501 so let's continue this conversation over there.

@xtuc xtuc referenced this issue Mar 10, 2018

Merged

March 13 meeting #58

@loganfsmyth loganfsmyth modified the milestones: Babel 7.next, Babel 8.x May 23, 2018

@loganfsmyth loganfsmyth changed the title Gain consensus on path forward for "sourceType" for parsing modules vs scripts in 7.x [8.x] Gain consensus on path forward for "sourceType" for parsing modules vs scripts May 23, 2018

@loganfsmyth

This comment has been minimized.

Copy link
Member Author

loganfsmyth commented May 23, 2018

Per #7501 (comment), we have decided to postpone this discussion until Babel 8.x.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.