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

💅 possible difference in scope between biome useliteralkeys and eslint dot-notation rules #3085

Closed
1 task done
scharkthemark opened this issue Jun 6, 2024 · 5 comments · Fixed by #3111
Closed
1 task done
Assignees
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages

Comments

@scharkthemark
Copy link

Environment information

CLI:
  Version:                      1.7.3
  Color support:                true

Platform:
  CPU Architecture:             x86_64
  OS:                           windows

Environment:
  BIOME_LOG_DIR:                unset
  NO_COLOR:                     unset
  TERM:                         unset
  JS_RUNTIME_VERSION:           "v20.13.1"
  JS_RUNTIME_NAME:              "node"
  NODE_PACKAGE_MANAGER:         "npm/10.5.2"

Biome Configuration:
  Status:                       Loaded successfully
  Formatter disabled:           false
  Linter disabled:              false
  Organize imports disabled:    false
  VCS disabled:                 true

Linter:
  Recommended:                  false
  All:                          false
  Rules:                        unset

Workspace:
  Open Documents:               0

Rule name

useLiteralKeys

Playground link

https://biomejs.dev/playground/?lineWidth=100&indentStyle=space&quoteProperties=preserve&indentWidth=4&code=YwBvAG4AcwB0ACAAdABlAHMAdAAgAD0AIAB7AAoAIAAgACAAIAAiAHQAZQBzAHQAIgA6ACAAMQAsAAoAIAAgACAAIAAiAHQAZQBzAHQAIAAyACIAOgAgADIALAAKACAAIAAgACAAIgB0AGUAcwB0ADMAIgA6ACAAMQAsAAoAfQA7AAoACgB0AGUAcwB0AFsAIgB0AGUAcwB0ACIAXQAgAD0AIAAyAA%3D%3D

Expected result

Hey folks,

There seems to be a possible difference between the scope of biome's useLiteralKeys linter rule and the scope of eslint's dot-notation linter rule. In particular, eslint seems to only apply the rule when the property of an object is being accessed, but biome seems to apply the rule both when the property is being accessed AND when the properties are being defined/created within a new object. For instance:

const test = {
    "hello": 1
}

In this example, eslint won't throw an error, but biome will and suggest to simplify by not using a string literal. You can view results in ESLint here. If that's the intended nature, all good, though would prefer parity with eslint as much as possible to help teams migrating; if that's not the intended nature, then there is probably a bug.

Thanks for your great work on this project, I love using it!

Code of Conduct

  • I agree to follow Biome's Code of Conduct
@Conaclos
Copy link
Member

Conaclos commented Jun 6, 2024

I believe this is intentional, but it does not seem to be properly documented.

I question this behavior because our formatter is able to remove unnecessary quotes around properties.
In fact, useLiteralKeys conflicts with the quoteProperties configuration of our JavaScript formatter.

So, personally, I am in favor of reducing the scope of useLiteralKeys to property access.

@Conaclos Conaclos added A-Linter Area: linter L-JavaScript Language: JavaScript and super languages labels Jun 6, 2024
@ematipico
Copy link
Member

So, personally, I am in favor of reducing the scope of useLiteralKeys to property access.

It's not very clear what reducing scope means. If I understand correctly, the code action should only remove the brackets and keep the quotes, correct? E.g.

// original
const a = {
  ["f"]: b
}

New code action

const a = {
-  f: b
+  'f': b
}

@Conaclos
Copy link
Member

Conaclos commented Jun 6, 2024

It's not very clear what reducing scope means.

The rule could only apply to property access:

- a.b["c"];
+ a.b.c;

Your example could be untouched.
However, I noticed that our formatter doesn't remove brackets when they are not needed. This follows Prettier. Should we diverge from Prettier here? It could make sense?

@scharkthemark
Copy link
Author

scharkthemark commented Jun 6, 2024

In fact, useLiteralKeys conflicts with the quoteProperties configuration of our JavaScript formatter.

This is actually what tipped me off that something might be funky in the first place. I had set quoteProperties: "preserve" and ran it on this code:

const test = {
    "test": 1,
    "test 2": 2,
    "test3": 1,
};

The formatter preserved the quotes around the properties, but the linter threw errors. The only way to fix was to unquote the "test" and "test3" properties, which seems to contradict the intended nature of quoteProperties: "preserve"

Reducing the scope of useLiteralKeys to property access makes sense to me.

@ematipico
Copy link
Member

Yeah sounds good. We could consider this a fix, since it oversteps the formatter boundaries

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Linter Area: linter L-JavaScript Language: JavaScript and super languages
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants