Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

Support sourceType: "unambiguous" parsing #449

Closed
wants to merge 1 commit into from

Conversation

loganfsmyth
Copy link
Member

@loganfsmyth loganfsmyth commented Apr 5, 2017

Q A
Bug fix? N
Breaking change? N
New feature? Y
Deprecations?
Spec compliancy?
Tests added/pass? N
Fixed tickets #440
License MIT

This code is ugly so definitely consider a WIP, but I'm posting for feedback before I go any further. This is the same logic used by Webpack to parse files and will result in files being treated as modules if they contain at least one import/export statement.

My proposed usage of this in Babel itself would be to change Babel's current default sourceType: "module" to sourceType: path.extname(opts.filename || "") === ".mjs" ? "module" : "script" so that files that Babel can begin toggling module transpilation directly based on sourceType, and can throw errors if import statements are inserted into a CommonJS module for instance. Similarly, this means we can stop adding use strict at the top of CommonJS files automatically and stop rewriting this to undefined.

With this, users can opt into sourceType: unambiguous in their .babelrc to tell Babel to essentially guess what type of file it is. This same logic is already implemented in Webpack and a new other loaders.

src/index.js Outdated
child.type === "ImportDeclaration" ||
child.type === "ExportDeclaration" ||
child.type === "ExportAllDeclaration" ||
child.type === "ExportDefaultDeclaration"
Copy link
Member

Choose a reason for hiding this comment

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

An interesting case would be AwaitExpression -- though that probably just throws in the babylon stage instead, before even becoming an AwaitExpression.

await is reserved as a keyword at the top level (so not only in async functions), but only when the source type is module (Note 1 in https://tc39.github.io/ecma262/#prod-AwaitExpression).

Copy link
Member Author

Choose a reason for hiding this comment

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

You still get an AwaitExpression either way, I think that part of the spec is more to make like easier for parsers if they want.

In this example, if you use await as an identifier it'll throw and reparse as a script successfully, which I think is good?

Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to warn if ambiguous code is detected?

@codecov
Copy link

codecov bot commented Apr 5, 2017

Codecov Report

Merging #449 into master will decrease coverage by 0.44%.
The diff coverage is 5.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #449      +/-   ##
==========================================
- Coverage   98.26%   97.81%   -0.45%     
==========================================
  Files          20       20              
  Lines        3511     3527      +16     
  Branches      934      940       +6     
==========================================
  Hits         3450     3450              
- Misses         22       37      +15     
- Partials       39       40       +1
Flag Coverage Δ
#babel 81.96% <5.88%> (-0.38%) ⬇️
#babylon 96.56% <5.88%> (-0.45%) ⬇️
Impacted Files Coverage Δ
src/index.js 38.46% <5.88%> (-61.54%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed452b6...0a72b73. Read the comment docs.

src/index.js Outdated

// Rather than try to parse as a script first, we opt to parse as a module and convert back
// to a script where possible to avoid having to do a full re-parse of the input content.
if (!hasModuleSyntax(ast)) ast.program.sourceType = "script";
Copy link
Member

@Jessidhia Jessidhia Apr 5, 2017

Choose a reason for hiding this comment

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

The "correct logic" would be to parse as script first and module later if that fails, but true, it's a lot more likely to get code that parses as both (or module only) as input. Not a lot of people still have code that only parses in sloppy mode, even if they still use sloppy mode semantics.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think it's as long as the output result is the same, the potential better average performance makes me want to go this route.

Copy link
Member

@jdalton jdalton Apr 6, 2017

Choose a reason for hiding this comment

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

For perf I had the acorn folks dig into proofing out an optimized top level parse.
In reify we also do a source sniff for import or export and if it fails we skip it.

@danez
Copy link
Member

danez commented Apr 5, 2017

Will node do the same now?
If extension is mjs then assume module otherwise autodetect?
Did they finally agree on something?

@Jessidhia
Copy link
Member

Nope! This is still a best guess. The only thing that's certain is that if mjs happens that it'll always be parsed as a module.

@loganfsmyth
Copy link
Member Author

I think at this point Node will end up with .mjs for modules and .js for CommonJS. That'll be a slow transition for the community though, so I think it's reasonable to force module for .mjs and use this for .js for a while at least. It'll also keep us more consistent with Webpack in the short term and allow the community to migrate over on their own time.

@loganfsmyth loganfsmyth force-pushed the unambiguous-parsing branch 2 times, most recently from f4fe97d to 7ccf0d8 Compare October 3, 2017 23:08
@loganfsmyth loganfsmyth changed the title WIP: Example sourceType: "unambiguous" parsing Support sourceType: "unambiguous" parsing Oct 25, 2017
@danez
Copy link
Member

danez commented Nov 1, 2017

We moved babylon back into the monorepo at https:/babel/babel/tree/master/packages/babylon.

Unfortunately pull requests cannot be migrated between repositories automatically. If this PR is still valid please reopen it there.
Thanks for your effort.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants