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

Preserve parens around expressions in default export declaration that start with function or class #6133

Merged
merged 12 commits into from
May 20, 2019

Conversation

duailibe
Copy link
Member

@duailibe duailibe commented May 18, 2019

@thorn0
Copy link
Member

thorn0 commented May 18, 2019

It's not something specific to as-expressions. As per the spec, the same happens with other default-exported expressions whose first operand starts with one of these keywords: function, async function, class. So this is not TypeScript-specific. E.g., Babel doesn't parse this too:

Prettier 1.17.1
Playground link

--parser typescript

Input:

export default function log(){} * 2;

Output:

SyntaxError: Expression expected. (1:33)
> 1 | export default function log(){} * 2;
    |                                 ^
  2 | 

@duailibe
Copy link
Member Author

@thorn0 Can you think of practical cases that aren't yet covered?

The one you mentioned is correct per the spec, but doesn't happen in practice because isn't valid in the runtime, so we shouldn't need to worry about that.

@thorn0
Copy link
Member

thorn0 commented May 19, 2019

It surely feels like a stretch, but still, I can imagine somebody doing things like these:

export default (function(){  /* ... */ }.toString());
export default (class WeirdSingleton { /* ... */ }.getInstance());

And don't forget about the upcoming pipeline operator. It totally makes sense for a function or a class expression to be its left-hand operand.

@duailibe
Copy link
Member Author

Thanks @thorn0, those are more practical use cases.

Unfortunately because of the format of the AST it's a bit hard to detect when there's a function or class keyword in the start of the expression.

We do have some code to try and guess what is the "left most" node in an expression to add ASI protection. I'll see if we can reuse some of the code for that. Otherwise, we have to manually handle all possible cases.

@duailibe duailibe changed the title Preserve parens around TsAsExpression inside default export declaration Preserve parens around expressions in default export declaration that start with function or class May 19, 2019
@duailibe
Copy link
Member Author

@lydell @thorn0 What do you think of this solution now?

@thorn0
Copy link
Member

thorn0 commented May 19, 2019

Tagged template literals get extra parens:

Prettier pr-6133
Playground link

--parser babylon

Input:

export default (function() {})`a`;

Output:

export default ((function() {})`a`);

@duailibe
Copy link
Member Author

duailibe commented May 19, 2019

@thorn0 Thank you for testing!

Now it only wraps the expression being exported if the function/class isn't already wrapped.


exports[`function_in_template.js 1`] = `
====================================options=====================================
parsers: ["flow", "typescript"]
Copy link
Member

Choose a reason for hiding this comment

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

I wonder why these tests aren't run using babel.

return false;
}

return path.call.apply(
Copy link
Member

Choose a reason for hiding this comment

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

Why not use spread syntax here instead of apply and concat?

@thorn0
Copy link
Member

thorn0 commented May 20, 2019

Overall looks great, but I did manage to produce superfluous parens:

Prettier pr-6133
Playground link

--parser babel

Input:

export default (function(){} |> a)``;

Output:

export default ((function() {} |> a)``);

@duailibe
Copy link
Member Author

duailibe commented May 20, 2019

@thorn0 Thanks for stress testing!

After years of Prettier this is the first bug report we've had, I think we're on the safe side now. From a practical point of view, we've covered enough cases IMO.

@duailibe duailibe merged commit 7e47b4e into prettier:master May 20, 2019
@duailibe duailibe deleted the export-default-as branch May 20, 2019 12:31
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Aug 18, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Aug 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parens incorrectly dropped when trying to type a function expression
3 participants