Skip to content

chore: add ESLint config and commands for linting JavaScript bindings.#248

Merged
victor-linroth-sensmetry merged 4 commits intomainfrom
chore/eslint-for-js-bindings
Apr 1, 2026
Merged

chore: add ESLint config and commands for linting JavaScript bindings.#248
victor-linroth-sensmetry merged 4 commits intomainfrom
chore/eslint-for-js-bindings

Conversation

@victor-linroth-sensmetry
Copy link
Copy Markdown
Collaborator

No description provided.

Copy link
Copy Markdown
Collaborator

@andrius-puksta-sensmetry andrius-puksta-sensmetry left a comment

Choose a reason for hiding this comment

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

Add node requirement to JS readme, also explain how to run the linter there.

EDIT: though since node is required by npm anyway, we can leave this out.

@victor-linroth-sensmetry victor-linroth-sensmetry force-pushed the chore/eslint-for-js-bindings branch from db0db0d to ef552ce Compare March 31, 2026 09:53
Copy link
Copy Markdown
Collaborator

@consideRatio consideRatio left a comment

Choose a reason for hiding this comment

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

I pushed a commit to lean on pre-commit / prek and the pre-commit.ci service to run automated CI tests. Do you think this is okay @victor-linroth-sensmetry ?

I appreciate having prek catch issues before I even commit / push, and also to avoid burdening github runners which we are bottlenecking on whenever more than one PR is triggered.

Screencast.from.2026-03-31.14-39-18.webm

@victor-linroth-sensmetry
Copy link
Copy Markdown
Collaborator Author

@consideRatio I don't care too much about exactly how things are run in the CI, but I'd like to have the option to run things manually as well.

@consideRatio
Copy link
Copy Markdown
Collaborator

Okay nice, you can do that with prek run!

  • -a / --all-files checks everything, and not including it means it only checks what you currently have git staged and are about to commit

@victor-linroth-sensmetry
Copy link
Copy Markdown
Collaborator Author

Are we intending to make prek mandatory for development? I thought it was meant to be optional.

@consideRatio
Copy link
Copy Markdown
Collaborator

Are we intending to make prek mandatory for development? I thought it was meant to be optional.

With the commit I made, it became the streamlined way to run eslint locally. I initially added alongside the adding of eslint to a devDependency + script in package.json, allowing for npm install + npm run lint to work.

I failed to avoid needing to duplicate eslint/globals dependencies then, and figured it was better to remove them from package.json and lean on prek + pre-commit.ci at that point, to avoid maintenance burden updating two sets of deps, and also avoiding having the github CI tests install eslint without using it.

Should we add back the additions to package.json and document npm run lint as well?

@victor-linroth-sensmetry
Copy link
Copy Markdown
Collaborator Author

Should we add back the additions to package.json and document npm run lint as well?

If we're not the point where we want to make prek the central runner of all checks, then I think it should still be there. Maybe we want to reevaluate that later, but it doesn't seem like we're there yet.

@consideRatio
Copy link
Copy Markdown
Collaborator

I'd like pre-commit.ci to be used for anything related to lint/formatting, as it then aligns with doing it for prek/pre-commit locally.

You see value in having npm run lint along the side - adding it back!

@victor-linroth-sensmetry
Copy link
Copy Markdown
Collaborator Author

Why was the config changed from ESM to CommonJS? ESM seems like the default choice for ESLint and the package is of type module.

@consideRatio
Copy link
Copy Markdown
Collaborator

Why was the config changed from ESM to CommonJS? ESM seems like the default choice for ESLint and the package is of type module.

It was dor compatibility with prek/pre-commit, which installs node dependencies in other places than directly under a node modules subfolder. With .cjs file, a node path env var is respected, but not with a .mjs file.

victor-linroth-sensmetry and others added 4 commits April 1, 2026 08:32
Signed-off-by: victor.linroth.sensmetry <victor.linroth@sensmetry.com>
…ning chores

Signed-off-by: victor.linroth.sensmetry <victor.linroth@sensmetry.com>
Signed-off-by: Erik Sundell <erik.sundell+2025@sensmetry.com>
This is provided alongside pre-commit doing it as well.

Signed-off-by: Erik Sundell <erik.sundell+2025@sensmetry.com>
@victor-linroth-sensmetry victor-linroth-sensmetry force-pushed the chore/eslint-for-js-bindings branch from a9099fb to 3daa21a Compare April 1, 2026 06:33
@victor-linroth-sensmetry victor-linroth-sensmetry merged commit 8fbda0c into main Apr 1, 2026
33 checks passed
@victor-linroth-sensmetry victor-linroth-sensmetry deleted the chore/eslint-for-js-bindings branch April 1, 2026 06:35
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.

3 participants