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

Fix formatting on JsxElement/JsxExpression #9361

Merged
merged 7 commits into from
Jun 28, 2016

Conversation

saschanaz
Copy link
Contributor

@saschanaz saschanaz commented Jun 26, 2016

Fixes #8817, fixes #6716

Input:

<div>,{integer}</div>;
<div>,   {integer}</div>;
<span>)</span>;
<span>)   </span>;
<Router routes={                3    } />;
<Router routes={                (3)    } />;

Current:

<div>, {integer}</div>;
<div>, {integer}</div>;
<span>) </span>;
<span>) </span>;
<Router routes={                3    } />;
<Router routes={                (3) } />;

Fix:

<div>,{integer}</div>;
<div>,   {integer}</div>;
<span>)</span>;
<span>)   </span>;
<Router routes={3} />;
<Router routes={(3)} />;

With new InsertSpaceAfterOpeningAndBeforeClosingJsxExpressionBraces option:

<div>,{ integer }</div>;
<div>,   { integer }</div>;
<span>)</span>;
<span>)   </span>;
<Router routes={ 3 } />;
<Router routes={ (3) } />;

@jwbay
Copy link
Contributor

jwbay commented Jun 26, 2016

Would this change
<Router routes={ 3 } />;
to
<Router routes={3} />; ?

My team has adopted the first via a lint rule because the spacing makes it easier to read (subjective opinion, I know). If the TS formatter starts enforcing the second, well, there goes that 😅

@saschanaz
Copy link
Contributor Author

@jwbay So we need an additional formatting option. Thanks for the information 👍

@saschanaz
Copy link
Contributor Author

Now we have the option!

@RyanCavanaugh
Copy link
Member

👍 greatly appreciated! 🏆

@@ -1267,6 +1267,7 @@ namespace ts {
InsertSpaceAfterOpeningAndBeforeClosingNonemptyParenthesis: boolean;
InsertSpaceAfterOpeningAndBeforeClosingNonemptyBrackets: boolean;
InsertSpaceAfterOpeningAndBeforeClosingTemplateStringBraces: boolean;
InsertSpaceAfterOpeningAndBeforeClosingJsxExpressionBraces: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be optional, or it is a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry but I'm not sure what you mean by 'optional', do you want me to make it insert?: boolean form?

@mhegazy
Copy link
Contributor

mhegazy commented Jun 27, 2016

Thanks @saschanaz. one comment about the optionality of the new format option, and we are good to go.

@chicoxyzzy
Copy link
Contributor

Does it also fix #6716 (comment)?

@zhengbli
Copy link
Contributor

@chicoxyzzy Yes it does.

@zhengbli
Copy link
Contributor

@saschanaz Can you update the PR today? As we are getting closer to snap 2.0, this needs to get in soon

@saschanaz
Copy link
Contributor Author

saschanaz commented Jun 28, 2016

@zhengbli I can if I get the answer for #9361 (comment). Not sure what 'optional' means here as any other fields on FormatCodeOptions are not optional.

@mhegazy
Copy link
Contributor

mhegazy commented Jun 28, 2016

Not sure what 'optional' means here as any other fields on FormatCodeOptions are not optional.

they were not optional originally, and that was a mistake. they should all have been optional from the get go.
Now adding a new one will break the build for any tool that uses formatting, even if they do not care to support this new option. so newer ones should be optional, and possibly we should go back and make all the options optional (no pun intended).

@saschanaz
Copy link
Contributor Author

I think making it optional will make me forget adding corresponding fields on fourslash and editorServices for new default option value. Well, that is another story anyway ;)

@mhegazy mhegazy merged commit ec02077 into microsoft:master Jun 28, 2016
@mhegazy
Copy link
Contributor

mhegazy commented Jun 28, 2016

thanks as always @saschanaz!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

JSX formatting inside of tag TSX: Space is added before closing parenthesis
7 participants