Skip to content

Chore/upgrade eslint #7853

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

Draft
wants to merge 3 commits into
base: dev-2.0
Choose a base branch
from

Conversation

error-four-o-four
Copy link
Contributor

Changes:

  • Bump eslint from 8.54.0 to 9.27
  • Introduce @sytlistic/eslint-plugin to replace deprecated eslint rules
  • Sort .gitignore
  • Disable all rules to avoid breaking actions/workflows

@error-four-o-four
Copy link
Contributor Author

error-four-o-four commented May 31, 2025

@limzykenneth

We have a situation ...

✖ 8271 problems (8250 errors, 21 warnings)
  6597 errors and 21 warnings potentially fixable with the `--fix` option.

... and I have a few questions:

  • Could you please confirm that the old rules are configured as intended? This might be a good opportunity to doublecheck and adjust the rules before they're applied.
  • Do you consider using other plugins like eslint-plugin-import-x, eslint-plugin-n, jsdoc, @eslint/json, @eslint/markdown? To prevent cluttering the dev dependencies one could also consider using a preconfigured config like @antfu/eslint-config
  • why? 😅
  • the old config targeted ES2022. Should this still be the case?
  • I'd argue to unignore .vscode/settings.json to disable auto formatting on save. Otherwise it could become messy

Next steps:
[ ] doublecheck ignored files
[ ] doublecheck the config regarding the files in ./test
[ ] enable rules separately and create a commit for each
[ ] adjust lint-staged config

Additionaly I have to wrap my head around ./utils/sample-linter.mjs and doublecheck it's implementation.

Cheers!

@limzykenneth
Copy link
Member

Could you please confirm that the old rules are configured as intended? This might be a good opportunity to doublecheck and adjust the rules before they're applied.

The existing rules should be all good and intended. The one thing to possibly consider is to switch from errors to warnings for the style related rules, eg. indentation and semi-colon, that does not change behavior of the code (ie. arrow function will still error since it change behavior of function more significantly). If there are some suggestion on rules changes or addition, do feel free to make them here and we can review.

Do you consider using other plugins like eslint-plugin-import-x, eslint-plugin-n, jsdoc, @eslint/json, @eslint/markdown? To prevent cluttering the dev dependencies one could also consider using a preconfigured config like @antfu/eslint-config

If needed, the additional plugins can be added to enable relevant linting of existing rules. I would like to limit the scope of linting to JS files only though so not really considering additional plugin to lint other file types.

why? 😅

I assume this is about why Typr.js is inlined in this repo? The reason is that the original Typr.js repo does not export the code as an NPM package so as a stop gap we inline the library files directly. It is not meant to be edited by p5.js contributors so this file can be ignored by ESLint.

the old config targeted ES2022. Should this still be the case?

It should be fine, unless in the 2.0 codebase it looks like we are using newer syntax then we can bump the version number up to 2024.

I'd argue to unignore .vscode/settings.json to disable auto formatting on save. Otherwise it could become messy

Yes should be fine.

@error-four-o-four
Copy link
Contributor Author

Could you please confirm that the old rules are configured as intended? This might be a good opportunity to doublecheck and adjust the rules before they're applied.

The existing rules should be all good and intended. The one thing to possibly consider is to switch from errors to warnings for the style related rules, eg. indentation and semi-colon, that does not change behavior of the code (ie. arrow function will still error since it change behavior of function more significantly). If there are some suggestion on rules changes or addition, do feel free to make them here and we can review.

I was wondering which would be the preferred way to integrate the linting rules?

As already mentioned I could create a commit for each rule
or
one could merge this PR which only includes the general config setup (when it's finished) and create a seperate PR for each rule. The latter provides opportunities for other people to contribute and might be more organized and uncluttered.
I'd honestly prefer the latter

why? 😅

I assume this is about why Typr.js is inlined in this repo? The reason is that the original Typr.js repo does not export the code as an NPM package so as a stop gap we inline the library files directly. It is not meant to be edited by p5.js contributors so this file can be ignored by ESLint.

Sorry if this sounded kind of impolite. I just wanted to know if this could be a task for the future? My aforementioned general intention is to help improve performance and reduce bundle size. See #7761

Do you consider using other plugins like eslint-plugin-import-x, eslint-plugin-n, jsdoc, @eslint/json, @eslint/markdown? To prevent cluttering the dev dependencies one could also consider using a preconfigured config like @antfu/eslint-config

If needed, the additional plugins can be added to enable relevant linting of existing rules. I would like to limit the scope of linting to JS files only though so not really considering additional plugin to lint other file types.

I've been looking into utils/smaple-linter.mjs. As far as I can tell the purpose of the script is to lint the examples in the jsdoc comments and check the correct usage of the global methods and properties, right?
This is were eslint-plugin-jsdoc might come in really useful. The plugin provides a preprocessor and linting of js snippets in jsdoc comments by default. I'd suggest to generally inegrate it into the root config and drop the script.

Do you consider using other plugins like eslint-plugin-import-x, eslint-plugin-n, jsdoc, @eslint/json, @eslint/markdown? To prevent cluttering the dev dependencies one could also consider using a preconfigured config like @antfu/eslint-config

If needed, the additional plugins can be added to enable relevant linting of existing rules. I would like to limit the scope of linting to JS files only though so not really considering additional plugin to lint other file types.

The plugins could be used to lint js code snippets in .md and .html files. Should I implement this feature?

@limzykenneth
Copy link
Member

I was wondering which would be the preferred way to integrate the linting rules?

As already mentioned I could create a commit for each rule
or
one could merge this PR which only includes the general config setup (when it's finished) and create a seperate PR for each rule. The latter provides opportunities for other people to contribute and might be more organized and uncluttered.
I'd honestly prefer the latter

Instead of individual PR for each rule, which I feel is a bit excessive, we can do this PR as you mentioned with the general setup then open another unified PR (or start with an issue for discussion) for discussing the rules in details. That way we avoid spilling too much bike shedding conversation around and just have one place dedicated for it. What do you think?

Sorry if this sounded kind of impolite. I just wanted to know if this could be a task for the future? My aforementioned general intention is to help improve performance and reduce bundle size. See #7761

No worries, it is just worth to be a bit clearer on the question so I don't have to assume. For things like Typr.js, it might be worth discussing this with @dhowe who worked on the new implementation of the typography section replacing opentype.js with Typr.js as well. There are some existing discussion issues around too so might want to have a look at what's already looked at in those first.

I've been looking into utils/smaple-linter.mjs. As far as I can tell the purpose of the script is to lint the examples in the jsdoc comments and check the correct usage of the global methods and properties, right?
This is were eslint-plugin-jsdoc might come in really useful. The plugin provides a preprocessor and linting of js snippets in jsdoc comments by default. I'd suggest to generally inegrate it into the root config and drop the script.

Yes that is meant to lint the examples so that they also follow the lint rules we have. If the eslint-plugin-jsdoc can lint example code, that would really help us not having the custom solution we have that is probably more brittle, so yes do look into it.

The plugins could be used to lint js code snippets in .md and .html files. Should I implement this feature?

That could be helpful 🤔 I think maybe let's have the md plugin, I don't think we have much complex html with contents in the repo.

@error-four-o-four
Copy link
Contributor Author

Instead of individual PR for each rule, which I feel is a bit excessive, we can do this PR as you mentioned with the general setup then open another unified PR (or start with an issue for discussion) for discussing the rules in details. That way we avoid spilling too much bike shedding conversation around and just have one place dedicated for it. What do you think?

sounds good to me

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

Successfully merging this pull request may close these issues.

2 participants