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

Unexpected Strings must use singlequote error on jsonc/auto rule? #178

Closed
JounQin opened this issue Jul 2, 2022 · 11 comments · Fixed by #180
Closed

Unexpected Strings must use singlequote error on jsonc/auto rule? #178

JounQin opened this issue Jul 2, 2022 · 11 comments · Fixed by #180

Comments

@JounQin
Copy link
Collaborator

JounQin commented Jul 2, 2022

```json
{
  "plugins": [
    "preset-lint-consistent",
    "preset-lint-recommended",
    "preset-lint-markdown-style-guide",
    "preset-prettier"
  ]
}
```

JSON in markdown, parsed by eslint-mdx with mdx/code-blocks setting enabled.

My related eslint config: https://github.com/1stG/configs/blob/master/packages/eslint-config/overrides.js#L490-L513 and prettier-config: https://github.com/1stG/configs/blob/master/packages/prettier-config/base.js

It is processed as x.md/0_x.json as virtual filename.

@JounQin
Copy link
Collaborator Author

JounQin commented Jul 2, 2022

related codes, virtual filenames should be considered, there is a context.getPhysicalFilename() API get the filename directly, and when a virtual filename is being used, it should be used to resolve ESLint config directly.

I'm going to work on a fix if you agree with above.

@ota-meshi

@JounQin
Copy link
Collaborator Author

JounQin commented Jul 2, 2022

And, jsonc/auto seems can not be disabled once enabled, if true, it should be documented because this behavior is definitely unsafe.

Also, it is using another ESLint CLIEngine inside on every single call, this would affect the performance implicitly, this should also be documented. This rule should be discouraged IMO.

@JounQin JounQin changed the title Unexpected Strings must use singlequote error? Unexpected Strings must use singlequote error on jsonc/auto rule? Jul 2, 2022
@ota-meshi
Copy link
Owner

I haven't yet identified the cause of this issue. I'm having a hard time reproducing the problem. Can you provide a reproducible repository?

@ota-meshi
Copy link
Owner

related codes, virtual filenames should be considered, there is a context.getPhysicalFilename() API get the filename directly, and when a virtual filename is being used, it should be used to resolve ESLint config directly.

I think we probably can't use getPhysicalFilename() because we need to get the configuration of *.json 🤔

@JounQin
Copy link
Collaborator Author

JounQin commented Jul 2, 2022

https://github.com/1stG/configs/tree/repro/jsonc

Here we go.

# branch repro/jsonc

yarn && yarn eslint README.md

I think we probably can't use getPhysicalFilename() because we need to get the configuration of *.json 🤔

I mean if context.getPhysicalFilename() != context.getFilename(), then it is a virtual filename, we should use context.getFilename() without fallback to get its configuration, I don't know will it work as expected.

@JounQin
Copy link
Collaborator Author

JounQin commented Jul 3, 2022

And, jsonc/auto seems can not be disabled once enabled, if true, it should be documented because this behavior is definitely unsafe.

Also, it is using another ESLint CLIEngine inside on every single call, this would affect the performance implicitly, this should also be documented. This rule should be discouraged IMO.

@ota-meshi What do you think about these comments?

@ota-meshi
Copy link
Owner

CLIEngine is used only when using eslint v6 together, so I think it's okay if we drop supporting eslint v6.
What do you think?

@ota-meshi
Copy link
Owner

ota-meshi commented Jul 3, 2022

Also, the part where wanted to use getPhysicalFilename is the code used for eslint v6, so getPhysicalFilename does not exist yet.

@JounQin
Copy link
Collaborator Author

JounQin commented Jul 3, 2022

And, jsonc/auto seems can not be disabled once enabled

I'm not sure if it is true, when I met this bug I tried to disable rule jsonc/auto but failed.

For other ESLint versions, new eslintrc.Legacy.CascadingConfigArrayFactory() is used, right? The Legacy symbol indicates we should not rely on it? And still, it will resolve configs for every single file, right?

@ota-meshi
Copy link
Owner

ota-meshi commented Jul 3, 2022

And, jsonc/auto seems can not be disabled once enabled

Hmm. I can't reproduce it. If the issue remains and you can reproduce the issue, could you share the detailed steps to reproduce it?

The Legacy symbol indicates we should not rely on it?

The ESLint team plans to implement a new configuration file format. That's why, I think the current configuration format is called Legacy. However, I have not confirmed it, so it may be wrong.

@ota-meshi
Copy link
Owner

I found a document explaining it.
https://github.com/eslint/eslintrc#future-usage

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants