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

Upgrade eslint 8 > 9 #5777

Open
wants to merge 7 commits into
base: development
Choose a base branch
from

Conversation

PikachuEXE
Copy link
Collaborator

@PikachuEXE PikachuEXE commented Oct 1, 2024

Pull Request Type

  • Bugfix
  • Feature Implementation
  • Documentation
  • Other

Related issue

Description

  • Can't use --ext anymore, and using blob like ./**/*.js is very slow compared to specific folders, so only 2 folders are specified for js/vue files
  • Separate config files for json/yaml files due to I can't avoid recommended config from being applied on them
  • Draft until [New] support eslint 9 import-js/eslint-plugin-import#2996 released

Screenshots

image

Testing

  • yarn run lint
  • yarn run lint-json
  • yarn run lint-yml

Desktop

  • OS:
  • OS Version:
  • FreeTube version:

Additional context

@PikachuEXE
Copy link
Collaborator Author

Since we can switch from standard to neostandard later maybe we can just upgrade to ESLint 9 first?

Copy link
Member

@ChunkyProgrammer ChunkyProgrammer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should definitely try to fix: Separate config files for json/yaml files due to I can't avoid recommended config from being applied on them

Since IDEs/code editors wont be able to highlight the issues

eslint.yaml.config.mjs Outdated Show resolved Hide resolved
eslint.json.config.mjs Outdated Show resolved Hide resolved

settings: {
'vue-i18n': {
localeDir: './static/locales/{en-US,en-GB,ar,bg,ca,cs,da,de-DE,el,es,es-AR,es-MX,et,eu,fa,fi,fr-FR,gl,he,hu,hr,id,is,it,ja,ko,lt,nb-NO,nl,nn,pl,pt,pt-BR,pt-PT,ro,ru,sk,sl,sr,sv,tr,uk,vi,zh-CN,zh-TW}.yaml',
Copy link
Member

@absidue absidue Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please import the list instead of hardcoding it, so that we don't have to maintain it in multiple places. Using import activeLocales from './static/locales/activeLocales.json' with { type: 'json' } and then calling .join(',') on the imported array should work.

According to the history drop down on https://nodejs.org/api/esm.html#import-attributes the with keyword is supported in Node 21+ and in the LTS versions 18.20+ and 20.10+, so we should be fine using it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With dynamic import, got warning (probably non issue assuming it eventually no longer Experimental

const { default: activeLocales } =
  await import('./static/locales/activeLocales.json', { with: { type: 'json' } })

image

Also static import would only cause syntax error with node 20.17.0
image
image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason it didn't work with the static import is because of the missing : between type and 'json'.

Copy link
Contributor

github-actions bot commented Oct 4, 2024

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

github-actions bot commented Oct 5, 2024

Conflicts have been resolved. A maintainer will review the pull request shortly.

@PikachuEXE
Copy link
Collaborator Author

@ChunkyProgrammer config files merged
Turns out localeDir needed in all config blocks (even for json/yaml

@PikachuEXE PikachuEXE marked this pull request as ready for review October 5, 2024 03:13
@FreeTubeBot FreeTubeBot enabled auto-merge (squash) October 5, 2024 03:13
@github-actions github-actions bot added the PR: waiting for review For PRs that are complete, tested, and ready for review label Oct 5, 2024
Copy link
Member

@absidue absidue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows CMD doesn't interpret single quotes, so it passes them through to eslint and then eslint fails to find files that start and end with single quotes. I've suggested changes to switch to double quotes instead.

package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
eslint.config.mjs Outdated Show resolved Hide resolved
eslint.config.mjs Outdated Show resolved Hide resolved
@PikachuEXE
Copy link
Collaborator Author

Updated~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: dependencies Pull requests that update a dependency file PR: waiting for review For PRs that are complete, tested, and ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants