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

Fix "module" external helpers output #6377

Merged
merged 4 commits into from
Oct 3, 2017

Conversation

loganfsmyth
Copy link
Member

Q                       A
Fixed Issues Fixes #6367
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added/Pass?
Spec Compliancy?
License MIT
Doc PR
Any Dependency Changes?

This is an alternative to #6367 that I tries to keep things a little simpler.

@babel-bot
Copy link
Collaborator

babel-bot commented Oct 3, 2017

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

@nicolo-ribaudo
Copy link
Member

I actually prefer this PR, since it removes the special-casing for keyword helpers.

@hzoo hzoo added the PR: Bug Fix 🐛 A type of pull request used for our changelog categories label Oct 3, 2017
Copy link
Member

@Andarist Andarist left a comment

Choose a reason for hiding this comment

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

For sure the overall code is simpler - I shouldnt be afraid restructuring the functions in build-external-helpers so much, it would certainly help to make my life simpler.

This however doesn't do good by keywordHelpers, in module they are not namespaced and we cannot keep function typeof(obj) {...} - need to rename it and export as renamed back:

export { _typeof as typeof }
function _typeof(obj) {...}

body.unshift(
t.exportNamedDeclaration(
null,
Object.keys(refs).map(name => {
Copy link
Member

Choose a reason for hiding this comment

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

havent thought about that, nice!

@Andarist
Copy link
Member

Andarist commented Oct 3, 2017

Also I have added this if statement in order to handle exported identifiers (such as temporalUndefined) more gracefully here - while it worked because exportName was undefined in that case and therefore different than expectec helper name (id.name), it could have been a little bit by luck and not sure if the original author meant it that way. Imho handling Identifier case there makes things more explicit and therefore better.

@loganfsmyth
Copy link
Member Author

loganfsmyth commented Oct 3, 2017

@Andarist Ahh, good point about typeof. I'll fix that.

I'm not sure I follow what you're saying about temporalUndefined though.

@loganfsmyth loganfsmyth merged commit 5ea54f6 into babel:master Oct 3, 2017
@loganfsmyth loganfsmyth deleted the external-module-helpers branch October 3, 2017 20:58
@Andarist
Copy link
Member

Andarist commented Oct 3, 2017

This is perfect! 🎉

@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: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants