This repository has been archived by the owner. It is now read-only.

Throw error when exporting non-declaration #241

Merged
merged 2 commits into from Dec 1, 2016

Conversation

Projects
None yet
4 participants
@kaicataldo
Copy link
Member

kaicataldo commented Nov 29, 2016

Q A
Bug fix? yes
Breaking change? yes
New feature? no
Deprecations? no
Spec compliancy? yes
Tests added/pass? yes
Fixed tickets #238
License MIT

Currently, Babylon will parse any export that starts with a keyword as a declaration. These changes will throw when the statement following the export keyword is not a declaration.

return this.parseStatement(true);
const statement = this.parseStatement(true);
if (!this.isDeclaration(statement)) {
this.unexpected(statement.start);

This comment has been minimized.

@DrewML

DrewML Nov 29, 2016

Member

Since we know what the problem is here (an export that doesn't have a valid declaration or export clause) maybe we can use this.raise and hint to the user what the problem was?

This comment has been minimized.

@kaicataldo

kaicataldo Nov 29, 2016

Author Member

After discussing a bit with @hzoo, I changed it to an ahead of time check rather than checking the export statement post-parse. This actually changes the error message in some cases, so wanted to see what do you think.

If we want a more descriptive error message I could leave the original conditional and do the shouldParseExportDeclaration check (maybe now called isDeclaration?) inside that block, throwing a more descriptive error if isDeclaration returns false.

My only concern with the error message is that I'm not sure we can be sure of what the intention of the code is. If Babylon is trying to parse export typeof foo, can we surmise they intended to export a declaration? Or did you mean a more generic "Not a valid export" type of message? Let me know if I'm missing something!

This comment has been minimized.

@DrewML

DrewML Nov 29, 2016

Member

Or did you mean a more generic "Not a valid export" type of message

Yep! Just some message indicating that we know it's an export, it's just not a valid one.

@kaicataldo

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Nov 29, 2016

Hmmm, actually, going to explore checking token values ahead of time rather than the node type afterwards

@kaicataldo

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Nov 29, 2016

Updated

@hzoo hzoo added the Tag: Bug Fix label Nov 29, 2016

@hzoo

hzoo approved these changes Nov 29, 2016

@kaicataldo kaicataldo force-pushed the kaicataldo:fixes238 branch from f3a054d to 2baffc4 Nov 30, 2016

@kaicataldo kaicataldo force-pushed the kaicataldo:fixes238 branch from 2baffc4 to fb3f48c Nov 30, 2016

@kaicataldo

This comment has been minimized.

Copy link
Member Author

kaicataldo commented Nov 30, 2016

Sorry, one last update - added return value flow type and moved the shouldParseExportDeclaration method back to where it was before I removed the other methods

@danez

This comment has been minimized.

Copy link
Member

danez commented Dec 1, 2016

Nice thanks

@danez danez merged commit 5fb4353 into babel:master Dec 1, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kaicataldo kaicataldo deleted the kaicataldo:fixes238 branch Dec 1, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.