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

Help with eslint upgrade #2272

Open
2 of 3 tasks
yurishkuro opened this issue Apr 20, 2024 · 14 comments
Open
2 of 3 tasks

Help with eslint upgrade #2272

yurishkuro opened this issue Apr 20, 2024 · 14 comments

Comments

@yurishkuro
Copy link
Member

yurishkuro commented Apr 20, 2024

#2271 is failing to bump eslint versions:

  1. We need to change our build to use Node 20 in .github/workflow/** and .nvmrc. It would be nice to enforce it via package.json.
  2. This error: ESLintIgnoreWarning: The ".eslintignore" file is no longer supported. Switch to using the "ignores" property in "eslint.config.js": https://eslint.org/docs/latest/use/configure/migration-guide#ignoring-files
  3. New error: [eslint ] Error: Could not find config file.
Baalekshan added a commit to Baalekshan/jaeger-ui that referenced this issue Apr 20, 2024
Signed-off-by: Baalekshan <baalekshan@gmail.com>
Baalekshan added a commit to Baalekshan/jaeger-ui that referenced this issue Apr 20, 2024
Signed-off-by: Baalekshan <baalekshan@gmail.com>
Baalekshan added a commit to Baalekshan/jaeger-ui that referenced this issue Apr 20, 2024
Signed-off-by: Baalekshan <baalekshan@gmail.com>
Baalekshan added a commit to Baalekshan/jaeger-ui that referenced this issue Apr 20, 2024
Signed-off-by: Baalekshan <baalekshan@gmail.com>
Baalekshan added a commit to Baalekshan/jaeger-ui that referenced this issue Apr 20, 2024
Signed-off-by: Baalekshan <baalekshan@gmail.com>
Baalekshan added a commit to Baalekshan/jaeger-ui that referenced this issue Apr 20, 2024
Signed-off-by: Baalekshan <baalekshan@gmail.com>
@Baalekshan
Copy link
Contributor

Baalekshan commented Apr 20, 2024

Hi @yurishkuro , unit-tests workflow is failing in my commit, how can i resolve

yurishkuro pushed a commit that referenced this issue Apr 20, 2024
## Which problem is this PR solving?
#2272 

## Description of the changes
- 

## How was this change tested?
- 

## Checklist
- [x] I have read
https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
- [x] I have signed all commits
- [ ] I have added unit tests for the new functionality
- [x] I have run lint and test steps successfully
  - for `jaeger`: `make lint test`
  - for `jaeger-ui`: `yarn lint` and `yarn test`

---------

Signed-off-by: Baalekshan <baalekshan@gmail.com>
@yurishkuro
Copy link
Member Author

yurishkuro commented Apr 20, 2024

@Baalekshan the lint step is still failing after upgrade with

[eslint       ] Error: Could not find config file.
[eslint       ]     at locateConfigFileToUse (/home/runner/work/jaeger-ui/jaeger-ui/node_modules/eslint/lib/eslint/eslint.js:350:21)
[eslint       ]     at async calculateConfigArray (/home/runner/work/jaeger-ui/jaeger-ui/node_modules/eslint/lib/eslint/eslint.js:385:49)
[eslint       ]     at async ESLint.lintFiles (/home/runner/work/jaeger-ui/jaeger-ui/node_modules/eslint/lib/eslint/eslint.js:815:25)
[eslint       ]     at async Object.execute (/home/runner/work/jaeger-ui/jaeger-ui/node_modules/eslint/lib/cli.js:500:23)
[eslint       ]     at async main (/home/runner/work/jaeger-ui/jaeger-ui/node_modules/eslint/bin/eslint.js:153:22)
[eslint       ] error Command failed with exit code 2.

@Baalekshan
Copy link
Contributor

@yurishkuro Is this a concern ? Since, it was safely merged without any errors in the workflow. I didn't find this error in the PR raised only faced this in my forked repo.

@yurishkuro
Copy link
Member Author

@Baalekshan the change we merged was good, but it did not resolve this ticket, because the upgrade is still failing, just on a different error.

@Baalekshan
Copy link
Contributor

Oh right, what could be done now. Should I try fixing these errors? If so what can I do.

@yurishkuro
Copy link
Member Author

The upgrade is trying to bump eslint from 8.x to 9.x, there could be some breaking changes causing this issue.

@Baalekshan
Copy link
Contributor

I will look into it

@tico88612
Copy link
Contributor

tico88612 commented Apr 20, 2024

ESlint 9.x won't find .eslintrc.js in default.

There are two difference ways to resolve this.

  1. Env ESLINT_USE_FLAT_CONFIG set false
  2. .eslintrc.js migrate to eslint.config.js

@tico88612
Copy link
Contributor

Unfortunately, there are still some packages that can't support flat config (eslint.config.js)

It may be necessary to re-filter the .eslintrc.js content.

@Baalekshan
Copy link
Contributor

So do you mean we can't migrate completely but also use .eslint.config.js partially?

@tico88612
Copy link
Contributor

@Baalekshan
If you want to migrate settings, some of the packages will not work. (e.g. eslint-config-airbnb)

Set the environment variable ESLINT_USE_FLAT_CONFIG to false if you want to keep the full settings. (But I don't know about the possibility of .eslintrc.js being deprecated at some time.)

We can follow what @yurishkuro decides to do.

@Baalekshan
Copy link
Contributor

@tico88612 Alright!

@Baalekshan
Copy link
Contributor

@yurishkuro what should we do ?

@yurishkuro
Copy link
Member Author

In the linked airbnb ticket there is some mentioning of compat workaround, maybe it could work. Otherwise we can wait till they support it. I don't see much point in using env var solution, if's probably temporary anyway.

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

No branches or pull requests

3 participants