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

Allow sourceType:unambiguous as a way to tell Babylon to guess the type. #6789

Merged
merged 2 commits into from
Nov 10, 2017

Conversation

loganfsmyth
Copy link
Member

Q                       A
Fixed Issues? Fixes #6695
Patch: Bug Fix?
Major: Breaking Change? N
Minor: New Feature? Y
Tests Added + Pass? Yes
Documentation PR
Any Dependency Changes?
License MIT

Repost of babel/babylon#449. Also added tweaks to option parsing so that the option passes through properly.

@babel-bot
Copy link
Collaborator

babel-bot commented Nov 10, 2017

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

@babel-bot
Copy link
Collaborator

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

@hzoo hzoo requested a review from jdalton November 10, 2017 04:23
@hzoo
Copy link
Member

hzoo commented Nov 10, 2017

Should add a readme change for the new option? @bmeck @Kovensky

@hzoo hzoo added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Nov 10, 2017
Copy link
Member

@hzoo hzoo left a comment

Choose a reason for hiding this comment

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

looks good, probably want a doc change in a few places

(!child.exportKind || child.exportKind === "value")) ||
child.type === "ExportDefaultDeclaration",
);
}
Copy link
Member

@jdalton jdalton Nov 10, 2017

Choose a reason for hiding this comment

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

👆 Neat! (I might borrow this helper)

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 this is basically to stay in line with babel/babylon#771 since Flowtype just indiscriminately allows mixing ES6 import/export and CommonJS and it seems fine to allow type import/export inside sourceType:script files.

Copy link
Member

@jdalton jdalton Nov 10, 2017

Choose a reason for hiding this comment

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

@loganfsmyth Who made your avatar!?

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it from Matt Cummings. His commissions are open again: https://twitter.com/EiffelArt/status/928017289416495105

} catch (scriptError) {}

throw moduleError;
}
Copy link
Member

Choose a reason for hiding this comment

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

👆 This aligns with my approach as well.

) {
throw new Error(
`.${key} must be "module", "script", "unambiguous", or undefined`,
);
Copy link
Member

@jdalton jdalton Nov 10, 2017

Choose a reason for hiding this comment

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

The alphabetical listing of these sourceTypes in the error text is satisfying!

@@ -91,3 +111,16 @@ function getParserClass(
}
return cls;
}

Copy link
Member

@jdalton jdalton Nov 10, 2017

Choose a reason for hiding this comment

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

Could hasModuleSyntax be simplified?

Is there ever a child.type of ImportDeclaration and friends
that don't have the accompanying importKind and exportKind values?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe when the flow parsing isn't enabled, they aren't set.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: modules outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion: Should we have sourceType: "auto"
4 participants