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

Typescript transpile SyntaxError: A trailing comma is not permitted after the rest element #249

Closed
iki opened this issue Jun 14, 2018 · 3 comments

Comments

@iki
Copy link

iki commented Jun 14, 2018

It's forbidden in ES, but allowed in TS, so prettier inserts the comma, see prettier/prettier#3586 and prettier/prettier#3528.

Happens with latest sucrase@3.0.0.

ERROR in ./src/components/X.tsx
Module build failed (from ../node_modules/@sucrase/webpack-loader/dist/index.js):
SyntaxError: Error transforming ./src/components/X.tsx: A trailing comma is not permitted after the rest element (1:1032)
    at raise (./node_modules/sucrase/dist/parser/traverser/base.js:24:15)
    at unexpected (./node_modules/sucrase/dist/parser/traverser/util.js:63:25)
    at parseObj (./node_modules/sucrase/dist/parser/traverser/expression.js:719:32)
    at parseBindingAtom (./node_modules/sucrase/dist/parser/traverser/lval.js:63:32)
    at parseMaybeDefault (./node_modules/sucrase/dist/parser/traverser/lval.js:131:5)
    at parseAssignableListItem (./node_modules/sucrase/dist/parser/traverser/lval.js:115:3)
    at parseBindingList (./node_modules/sucrase/dist/parser/traverser/lval.js:104:7)
    at parseFunctionParams (./node_modules/sucrase/dist/parser/traverser/statement.js:579:30)
    at parseParenAndDistinguishExpression (./node_modules/sucrase/dist/parser/traverser/expression.js:608:42)
    at parseExprAtom (./node_modules/sucrase/dist/parser/traverser/expression.js:477:24)
    at parseExprSubscripts (./node_modules/sucrase/dist/parser/traverser/expression.js:258:20)
    at parseMaybeUnary (./node_modules/sucrase/dist/parser/traverser/expression.js:244:20)
    at parseExprOps (./node_modules/sucrase/dist/parser/traverser/expression.js:184:20)
    at parseMaybeConditional (./node_modules/sucrase/dist/parser/traverser/expression.js:157:20)
    at baseParseMaybeAssign (./node_modules/sucrase/dist/parser/traverser/expression.js:141:20)
    at tsParseMaybeAssign (./node_modules/sucrase/dist/parser/plugins/typescript.js:1306:49)
@alangpierce
Copy link
Owner

@iki Hmm, I think it's a bug in Prettier if it's inserting a trailing comma. TypeScript 2.9 had a breaking change to disallow trailing commas after rest elements ( https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#trailing-commas-not-allowed-on-rest-parameters ) and the latest playground shows an error ( https://www.typescriptlang.org/play/#src=const%20%7B%20...rest%2C%20%7D%20%3D%20%7B%7D%3B%0D%0A ). I'd prefer to stick with the latest TypeScript syntax rules unless there's a really compelling reason.

There's some more discussion here: prettier/prettier#4624 , although I think that's for a separate TS parsing bug that has since been fixed.

@iki
Copy link
Author

iki commented Jun 15, 2018

True, it changes with TS 2.9.

However, it's not easy on all projects to switch to 2.9. It needs updating code and some typed dependencies to compile correctly, so we can't use the sucrase powered webpack-serve on most atm.

Would you consider making that an option similar to enableLegacyTypeScriptModuleInterop?

@alangpierce
Copy link
Owner

I think it should be reasonable to just allow trailing commas here always without need for a new option. Generally Sucrase's philosophy is to skip error validation and leave that to the linter, typechecker, or JS runtime. For example, Babel doesn't let you export the same name twice, but Sucrase does. As you can see from the error message, there's already an explicit check to disallow this case, so the main thing to do is to just remove that check and make sure the comma doesn't cause problems with later parsing.

I may want to have a more explicit range of supported TypeScript versions in the future and eventually release a semver-major update that drops support for this case, but for now, TS 2.9 is new enough that it makes sense to support older syntax.

alangpierce added a commit that referenced this issue Jun 25, 2018
Fixes #249

Nothing stops us from allowing it at parse time, and it's allowed in TS <2.9, so
we support it for now.
alangpierce added a commit that referenced this issue Jun 25, 2018
Fixes #249

Nothing stops us from allowing it at parse time, and it's allowed in TS <2.9, so
we support it for now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants