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

Quote and unquote number keys #8508

Merged
merged 19 commits into from Jun 18, 2020

Conversation

lydell
Copy link
Member

@lydell lydell commented Jun 6, 2020

Closes #6064
Closes #6132

Note: Flow parses number keys, but errors on them during type checking, so we don’t unquote number keys for the flow and babel-flow parsers.

https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVBjOA7AzgFzAA8wBeMAb1TDAAYAuMARlQF8g

  • I’ve added tests to confirm my change works.
  • (If changing the API or CLI) I’ve documented the changes I’ve made (in the docs/ directory)
  • (If the change is user-facing) I’ve added my changes to changelog_unreleased/*/pr-XXXX.md file following changelog_unreleased/TEMPLATE.md.
  • I’ve read the contributing guidelines.

Try the playground for this PR

@lydell lydell force-pushed the fix-quote-props-for-numbers branch from 446fa52 to 9be56ce Compare June 6, 2020 19:12
src/language-js/utils.js Outdated Show resolved Hide resolved
271 jsfmt.spec.js in flow-repo use only the flow parser.
18 use flow and babel.
Now, all of them use only flow.
@thorn0
Copy link
Member

thorn0 commented Jun 6, 2020

They also must not be quoted or unquoted in obect types:
Prettier pr-8508
Playground link

--parser typescript

Input:

var foo: { "2": boolean }

Output:

var foo: { 2: boolean };

@thorn0
Copy link
Member

thorn0 commented Jun 6, 2020

Hm. I might be wrong about object types...

@lydell
Copy link
Member Author

lydell commented Jun 6, 2020

@thorn0
Copy link
Member

thorn0 commented Jun 6, 2020

Not always, but sometimes it does:

https://www.typescriptlang.org/play/#code/DYUwLgBAhhC8EG8IEYBcKA0EBM7sQF8AoUSAIzkQgHJlr1ktrt6dCiiATEAY2CgBOICKWhR0AaxABPAPYAzCGGkAHEAugBuLr35CR4CGTKSZG5Wo1ltRKDHjHtQA

So it's not safe to do in TS at all. Even in object literals.

@thorn0
Copy link
Member

thorn0 commented Jun 6, 2020

BigInts can be used as keys too: { 2n: 1 } gives the same result as { '2': 1 }.

@lydell
Copy link
Member Author

lydell commented Jun 6, 2020

I wonder if the default "babel" parser is starting to become dangerous … it supports some type syntax, but is not entirely safe to run on Flow and TypeScript code 🤔

@thorn0
Copy link
Member

thorn0 commented Jun 6, 2020

There is an issue about this #8204

@lydell
Copy link
Member Author

lydell commented Jun 7, 2020

Quoting and unquoting can be a little bit “destructive” (but still correct).

x = {
  0b10: null,
};

edit ->

x = {
  0b10: null,
  "something-else": null,
};

prettier ->

x = {
  "2": null,
  "something-else": null,
};

edit ->

x = {
  "2": null,
};

prettier ->

x = {
  2: null,
};

It’s like Prettier has turned 0b10 into 2!

Maybe we should only quote/unquote ^\d+$ 🤔
Or the above case is rare and fine…

@thorn0
Copy link
Member

thorn0 commented Jun 7, 2020

Quoting happens only if the user opts in for it using --quote-props consistent, so I think it's fine. As for unquoting, I think these cases are confusing and probably shouldn't be unescaped:
Prettier pr-8508
Playground link

--parser babel

Input:

a={'1e+100':1,'2e-10':2}

Output:

a = { 1e100: 1, 2e-10: 2 };

@lydell
Copy link
Member Author

lydell commented Jun 7, 2020

But I think it’s important that Prettier is “round-trippable”. An addition followed by a deletion should end up with the input code.

a = { 1e100: 1, 2e-10: 2 };

edit ->

a = { 1e100: 1, 2e-10: 2, "a-a": 3 };

prettier ->

a = { "1e+100": 1, "2e-10": 2, "a-a": 3 };

edit ->

a = { "1e+100": 1, "2e-10": 2 };

prettier – here we need to unqoute to get back again! ->

a = { 1e100: 1, 2e-10: 2 };

But that has the unfortunate side effect of "1e+100" being unquoted for as-needed users as well.

That’s why I was thinking we should only do stuff with “digits-only” (maybe also floats?) keys – I’m thinking that’s the only likely case we’ll encounter in real code.

@lydell
Copy link
Member Author

lydell commented Jun 7, 2020

Given everything we’ve learned in this PR, it could also make sense to leave things the way the are and close the issue, leaving it up to the user to decide, or to use the ESLint quote-props rule.

@thorn0
Copy link
Member

thorn0 commented Jun 7, 2020

All considered, I vote for "digits only, including floats". Let's play it safe and keep non-decimals, number separators, bigints, etc. as is.

@lydell lydell force-pushed the fix-quote-props-for-numbers branch from e717ec2 to 83bbfd3 Compare June 13, 2020 10:07
@lydell
Copy link
Member Author

lydell commented Jun 13, 2020

Updated to only touch “simple” numbers.

src/language-js/utils.js Outdated Show resolved Hide resolved
@fisker
Copy link
Sponsor Member

fisker commented Jun 18, 2020

Note: Flow parses number keys, but errors on them during type checking, so we don’t unquote number keys for the flow and babel-flow parsers.

But, parser=babel supports flow.

const parse = createParse("parse", ["jsx", "flow"]);

Related: #8204 (comment)

@thorn0
Copy link
Member

thorn0 commented Jun 18, 2020

That's fine. The babel parser isn't safe for Flow anyway: https://prettier.io/blog/2019/01/20/1.16.0.html#flow
I agree that it's a mess. BTW, probably this means that if we proceed with #8204 (parse Flow syntax only if the pragma is present), it won't really be a breaking change as we'd break something that is already broken.

@thorn0 thorn0 merged commit 9184743 into prettier:master Jun 18, 2020
@lydell lydell deleted the fix-quote-props-for-numbers branch June 18, 2020 16:46
topocount added a commit to sourcecred/sourcecred that referenced this pull request Sep 10, 2020
In the latest release of Prettier, number keys are "unquoted" by
default. This is all fine and well, but Flow reacts poorly to number
literals as keys. The most diplomatic solution seems to be setting
"quoteProps" to "preserve":
	https://prettier.io/docs/en/options.html#quote-props

However, this precipitated an interesting error in jest, where it was
unhappy about the reaction and review state props being unquoted, both
in the flow type notation and the object literal itself. I recommend
reading the PR that implemented this change for more discussion:
prettier/prettier#8508

test plan: `yarn unit`, `yarn flow`, `yarn check-pretty`
topocount added a commit to sourcecred/sourcecred that referenced this pull request Sep 10, 2020
* Bump prettier from 2.0.5 to 2.1.1

Bumps [prettier](https://github.com/prettier/prettier) from 2.0.5 to 2.1.1.
- [Release notes](https://github.com/prettier/prettier/releases)
- [Changelog](https://github.com/prettier/prettier/blob/master/CHANGELOG.md)
- [Commits](prettier/prettier@2.0.5...2.1.1)

Signed-off-by: dependabot[bot] <support@github.com>

* Prettier: preserve quotes in object props

In the latest release of Prettier, number keys are "unquoted" by
default. This is all fine and well, but Flow reacts poorly to number
literals as keys. The most diplomatic solution seems to be setting
"quoteProps" to "preserve":
	https://prettier.io/docs/en/options.html#quote-props

However, this precipitated an interesting error in jest, where it was
unhappy about the reaction and review state props being unquoted, both
in the flow type notation and the object literal itself. I recommend
reading the PR that implemented this change for more discussion:
prettier/prettier#8508

test plan: `yarn unit`, `yarn flow`, `yarn check-pretty`

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Kevin Siegler <17910833+topocount@users.noreply.github.com>
@fisker fisker mentioned this pull request Sep 30, 2020
4 tasks
@brodybits
Copy link
Contributor

brodybits commented Sep 30, 2020

we don’t unquote number keys for the flow and babel-flow parsers

Quote & unquote with TypeScript seems to be considered "not safe" as well, as was noted in some comments added to the source code.

@brodybits brodybits mentioned this pull request Oct 1, 2020
2 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2021
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.

--quote-props=consistent doesn't add quotes around number but it is required for consistency
5 participants