-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Parenthesize export assignments if needed #19590
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
Conversation
src/compiler/factory.ts
Outdated
@@ -1982,7 +1982,7 @@ namespace ts { | |||
node.decorators = asNodeArray(decorators); | |||
node.modifiers = asNodeArray(modifiers); | |||
node.isExportEquals = isExportEquals; | |||
node.expression = expression; | |||
node.expression = parenthesizeBinaryOperand(SyntaxKind.EqualsToken, expression, /*isLeftSideOfBinary*/ false, /*leftOperand*/ undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I can tell, export default <expr>
binds as tightly as export = <expr>
, so this seems appropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think export default
may need its own parenthesization logic. Per the spec, export default
accepts AssigmentExpression but has a lookahead restriction for function
, async function
, and class
.
Basically, that means you need to parenthesize in the following cases:
- BinaryExpression of CommaToken
- CommaList (synthetic list of multiple comma expressions)
- FunctionExpression
- ClassExpression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
src/compiler/factory.ts
Outdated
@@ -1982,7 +1982,7 @@ namespace ts { | |||
node.decorators = asNodeArray(decorators); | |||
node.modifiers = asNodeArray(modifiers); | |||
node.isExportEquals = isExportEquals; | |||
node.expression = expression; | |||
node.expression = parenthesizeBinaryOperand(SyntaxKind.EqualsToken, expression, /*isLeftSideOfBinary*/ false, /*leftOperand*/ undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I think export default
may need its own parenthesization logic. Per the spec, export default
accepts AssigmentExpression but has a lookahead restriction for function
, async function
, and class
.
Basically, that means you need to parenthesize in the following cases:
- BinaryExpression of CommaToken
- CommaList (synthetic list of multiple comma expressions)
- FunctionExpression
- ClassExpression
export function parenthesizeDefaultExpression(e: Expression) { | ||
const check = skipPartiallyEmittedExpressions(e); | ||
return (check.kind === SyntaxKind.ClassExpression || | ||
check.kind === SyntaxKind.FunctionExpression || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is parenthesizing FunctionExpression
and ClassExpression
enough to stop them from being seen as HoistableDeclaration
/ClassDeclaration
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Once you add parens, you are no longer in a declaration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be. Anything that doesn't work that way is parsing incorrectly.
Fixes #19574.