Chore/upgrade eslint

[error-four-o-four]

Changes:

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

[error-four-o-four]

@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]

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]

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]

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]

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

[error-four-o-four]

@limzykenneth Sorry, I made a (git) mess. I shouldn't do any git operations when I'm about to go to bed. But I wanted to get this done. Please let me know if I should restart with a clean PR/branch.

The majority of warnings are reported from style related rules like comma-dangle, indent, semi and quote. In order to reduce the noise in future PRs I tried to apply these rules because they do not (or at least shouldn't) change the behaviour of the code. Nonetheless I reverted that commit.

This PR introduces eslint-plugin-jsdoc and @eslint/markdown which were aligned to the recommendations in Documentation Style Guide. This guide also recommends the Airbnb Style Guide which unfortunately is not compatible with eslint v9+ and uses deprecated rules. There is a compatible fork but I wouldn't recommend it because it inlcudes all the react configs and does not provide a base config. The Airbnb Config uses eslint-plugin-import which is quite useful, important, and necessary. But it's not compatible with v9+ either. I'd highly recommend eslint-plugin-import-x because while reviewing the changes made by the style related rules I noticed that @d4c/numjs/build/module/numjs.min.js is imported in src\math\Matrices\MatrixNumjs.js which is not a listed dependency.

I haven't touched the sample-linter yet because I can imagine different approaches about how to implement it's behaviour. On one hand one could keep it as a seperate script but use the jsdoc preprocessor and on the other hand one could inlcude it as an eslint config/rule. As far as I can tell the latter approach requires docs/reference/data.min.json and the types being generated but this file is gitignored and the scripts which generate them is not part of the final build script and would fail in CI/CD, right?. Is there a particular reason why it's ignored (except for the size of it)? The script generate-types could create another file like utils/p5Globals.js. The advantage of this approach would be that the p5 global methods used in the examples and tests would be linted in the IDE and the rule no-undef could be enabled.

This leads me to the ignored files. The eslint config of this PR ignores the files in test/ except for the unit tests because as mentioned above test/js and test/manual-test-examples depends on the declarations of globals.d.ts. Apart from that the helpers expect and assert are used inconsistently in color_conversion.js and p5.RendererGL.js

Another ignored file in 1.x is src/core/reference.js. I'm uncertain about what to do with it? This also applies to test/js/modernizr.js. At the moment the former is included in the esm build. Is this intended?

Oh dear, sorry for the wall of words but I just wanted to give you an update.

[limzykenneth]

@error-four-o-four Thanks for the updates

For the commit history don't worry too much about it, git will take care of it when merging and the diff still looks clean so should be fine.

For rules, ignore the Airbnb guide for now and just base things on the current rule set in the old config file, we'll discuss exact rules to adopt in a separate issue as mentioned last time. I'm not sure what eslint-plugin-import-x does exactly, I thought ESLint should be able to parse and lint import/export statements already or does this somehow expand its reach to other modules?

The Numjs Matrix file is just a work in progress, if it is causing problems you can add it to the ignore list.

If eslint-plugin-jsdoc can extract the example tags content and lint it we may not need any implementation of sample-linter.js. However the examples are currently inlined with HTML tags, I think this can be refactored out with changes to the website (which can be completed after this PR is done if needed).

The script generate-types could create another file like utils/p5Globals.js. The advantage of this approach would be that the p5 global methods used in the examples and tests would be linted in the IDE and the rule no-undef could be enabled.

I think it would be an idea we can explore, may want to do this later though as a separate thing. Same with the tests, we need to normalize it later.

reference.js is not used anymore in 2.x and I think so for modernizr.js as well, if you see it still being used in 2.x let me know and I can refactor.

[error-four-o-four]

Yes, eslint-plugin-import-x is able to check unused and unresolved imports. It also provides other useful rules but it might add overhead and reduce performance. Anyway this should also be discussed later.

I'll doublecheck this PR one more time this evening and then we shoud be good to go

[error-four-o-four]

For the commit history don't worry too much about it, git will take care of it when merging and the diff still looks clean so should be fine.

That was too risky for me. I made mistakes as I rebased stuff. So I started clean.

If eslint-plugin-jsdoc can extract the example tags content and lint it ...

Not yet but I've prepared it and will investigate this further.

We're good to go

https://github.com/processing/p5.js/actions/runs/15570657337/job/43845501279?pr=7853#step:5:4920

Thank you for your patience

[limzykenneth]

@lirenjie95 Thanks for helping to review!

@error-four-o-four I'm a bit swamped with work at the moment but I want to review this more thoroughly myself, so bear with me while I try to find some time to do so. Feel free to look at other things to work on if you like before I get back to this. Thanks!