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

Provide better error message for invalid default export declaration #7717

Merged

Conversation

dczombera
Copy link
Contributor

@dczombera dczombera commented Apr 12, 2018

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

I was not sure if an error message like the one suggested in the issue is consistent with the other error messages that babylon provides (is consistency even an issue when it comes to error messages?).
Apart from that, it's my first contribution to the project. I welcome any type of feedback. Thanks! :)

REPL from build

@babel-bot
Copy link
Collaborator

babel-bot commented Apr 12, 2018

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

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thank you for this PR!

) {
this.raise(
this.state.start,
"The `default` export can only take an expression;" +
Copy link
Member

Choose a reason for hiding this comment

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

It can also take function and class declarations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So would you suggest to just add both to the error message?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just:

Only expressions, functions or class declarations are allowed as the default export.

(Let's add tests for those too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I like that compact message.
So, I will change the message then and will also add tests for class declarations. Tests for functions declarations already exist as I've just seen.

@existentialism existentialism added PR: Polish 💅 A type of pull request used for our changelog categories pkg: parser labels Apr 12, 2018
@dczombera dczombera force-pushed the clear-message-on-default-export-error branch from 87e2343 to 9c4d0ef Compare April 12, 2018 20:18
@dczombera
Copy link
Contributor Author

dczombera commented Apr 12, 2018

Alright, I changed the error message as discussed and added tests for class declarations/expressions. ☺️

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.

Nice work, a great first PR!

@existentialism existentialism merged commit 61ec5ce into babel:master Apr 12, 2018
@existentialism
Copy link
Member

@dczombera thanks!

@jcurtis
Copy link

jcurtis commented Apr 12, 2018

This one always catches me off-guard (export default const). Thanks!

This might be the wrong forum but can anyone explain why you can't do that but you can do this?:

export const foo = 'bar';

@nicolo-ribaudo
Copy link
Member

An easy way to think about it is that named exports must have a name, while default exports not.

The real reason is that named exports export bindings, or variables (infact, if you modify an already exported variable the modules which import it will see the changes), while default exports export values.

@Jessidhia
Copy link
Member

But there's an asterisk... there's always an asterisk 😇

If your default export is a function or class declaration (no parens wrapping them), then those will be mutable bindings, and reassigning the function or class name will update the default export.

It's also possible to make anything you want the default export by using export { foo as default }.

@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 pkg: parser PR: Polish 💅 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

provide a remediation strategy when using export default with const, var, or let
7 participants