Skip to content
This repository has been archived by the owner on Sep 30, 2020. It is now read-only.

Change quote-props rule to "consistent-as-needed" #39

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wincent
Copy link
Contributor

@wincent wincent commented Mar 22, 2019

I just got bitten by this while preparing this PR.

Basically, given an object containing some version numbers properties:

{
  '6.2': true,
  '7.0': true,
  '7.1': true,
}

we currently have ESLint configured via "quote-props" to require quotes around property names "as-needed"; producing this:

{
  6.2: true,
  '7.0': true,
  7.1: true,
}

This looks pretty terrible. I noticed that Prettier didn't have an opinion on this, so I changed them all to skip the quotes:

{
  6.2: true,
  7.0: true,
  7.1: true,
}

of course, that is wrong too, because 7.0 actually gets stringified to '7', meaning the object is equivalent to:

{
  '6.2': true,
  '7': true,
  '7.1': true,
}

This breaks checks of the form if (object['7.0'] === true). So I added a lint suppression and put strings back on all the keys, but we should just configure ESLint to use "consistent-as-needed" instead. The docs) describe it like so:

requires quotes around all object literal property names if any name
strictly requires quotes, otherwise disallows quotes around object
property names

Test plan: tweak my local copy of eslint-config-liferay in the repo with the PR I linked at the top and confirm that all-quotes are enforced in that object but not in other places. It's not a total bed of roses, unfortunately. I found that if you start with an object like this:

{
  a: 1,
  'b-c': 2,
}

it will get changed to:

{
  "a": 1,
  'b-c': 2,
}

on the bright side, if you then change it to use single quotes, it stays that way. Closest ticket I can find for an issue like this is:

microsoft/vscode-eslint#209

So, it's a wrinkle, but I still think we're better off with this change than without it.

Closes: https://github.com/liferay/eslint-config-liferay/issues/38

I just got bitten by this while preparing this PR:

    liferay/liferay-js-themes-toolkit#247

Basically, given an object containing some version numbers properties:

```
{
  '6.2': true,
  '7.0': true,
  '7.1': true,
}
```

we currently have ESLint configured via "quote-props" to require quotes
waround property names "as-needed"; producing this:

```
{
  6.2: true,
  '7.0': true,
  7.1: true,
}
```

This looks pretty terrible. I noticed that Prettier didn't have an
opinion on this, so I changed them all to skip the quotes:

```
{
  6.2: true,
  7.0: true,
  7.1: true,
}
```

of course, that is wrong too, because `7.0` actually gets stringified to
`'7'`, meaning the object is equivalent to:

```
{
  '6.2': true,
  '7': true,
  '7.1': true,
}
```

This breaks checks of the form `if (object['7.0'] === true)`. So I added
a lint suppression and put strings back on all the keys, but we should
just configure ESLint to use "consistent-as-needed" instead. The
docs:

    https://eslint.org/docs/rules/quote-props):

describe it like so:

> requires quotes around all object literal property names if any name
> strictly requires quotes, otherwise disallows quotes around object
> property names

Test plan: tweak my local copy of eslint-config-liferay in the repo with
the PR I linked at the top and confirm that all-quotes are enforced in
that object but not in other places. It's not a total bed of roses,
unfortunately. I found that if you start with an object like this:

```
{
  a: 1,
  'b-c': 2,
}
```

it will get changed to:

```
{
  "a": 1,
  'b-c': 2,
}
```

on the bright side, if you then change it to use single quotes, it stays
that way. Closest ticket I can find for an issue like this is:

microsoft/vscode-eslint#209

So, it's a wrinkle, but I still think we're better off with this change
than without it.

Closes: https://github.com/liferay/eslint-config-liferay/issues/38
@wincent
Copy link
Contributor Author

wincent commented Mar 22, 2019

Note: don't want to merge this until Prettier 1.17 comes out, otherwise we'll have a conflict:

@wincent
Copy link
Contributor Author

wincent commented Apr 17, 2019

Prettier 1.17 is out, so I am going to try this out.

As noted in this comment:

#39 (comment)

we need the latest Prettier (1.17.0) in order to make use of the
"consistent-as-needed" rule, and in our Prettier config, we'll need to
use `"quote-props": "consistent"`.

So, I updated everything using `yarn upgrade-interactive` and added the
necessary rule to the ".prettierrc.json" file; other projects will need
to do the same.
@wincent
Copy link
Contributor Author

wincent commented Apr 17, 2019

Prettier 1.17 is out, so I am going to try this out.

Tried it, but there is still a bug, so probably best to hold off until it is resolved:

prettier/prettier#6064

@wincent
Copy link
Contributor Author

wincent commented Jun 18, 2020

At last, looks like we will be able to merge this with the next Prettier release, thanks to: prettier/prettier#8508

@jbalsas
Copy link
Contributor

jbalsas commented Jun 18, 2020

How did you remember about this?

@wincent
Copy link
Contributor Author

wincent commented Jun 19, 2020

I'm subscribed to the Prettier issue, so I go an email about it. Wasn't that hard to remember the connection. 😁

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.

Change quote-props rule to "consistent-as-needed"
2 participants