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

Old test is failing on master #1828

Closed
theoludwig opened this issue Jul 12, 2022 · 5 comments · Fixed by #1829
Closed

Old test is failing on master #1828

theoludwig opened this issue Jul 12, 2022 · 5 comments · Fixed by #1829

Comments

@theoludwig
Copy link
Member

The old test (GitHub Actions) on master is currently failing, it seems like a npm issue, not able to install dependencies.

I think it's worth removing this test as this doesn't really "test" the usage of StandardJS.
There was already a discussion about that in this PR: #1681

We can also remove the workaround with eval and import in bin/cmd.js, it makes the code ugly and I don't think it is worth it.
And instead to check the version with process.version, we can actually start using engines property in package.json (ref: standard/eslint-config-standard#211).

Basically bin/cmd.js becomes:

#!/usr/bin/env node

import * as standardEngine from 'standard-engine'

import options from '../options.js'

standardEngine.cli(options)

Thoughts?
Much cleaner, isn't it? Why adding code, we don't need and that actually break over time?

@voxpelli
Copy link
Member

I opted to extract this functionality into a dedicated module for two of my projects: https://github.com/voxpelli/version-guard

If we want to avoid maintaining the version guard then we can use that one here as well?

@theoludwig
Copy link
Member Author

Putting this logic in a dedicated module, only move the problem.

I think this is best to exit code 1 when a Node.js version is not supported or simply not care.

Also, it might be worth to add in the FAQ in the README.md, what Node.js versions are supported.

@voxpelli
Copy link
Member

The reason why its done like this is to allow easy upgrades to newer versions without it breaking projects that feel they need to support a wider set of Node.js versions than we do.

There is no reason for standard, dependency-check, installed-check or similar to fail when run on unsupported versions as that only forced those projects who have big test matrixes in their CI to either separate those projects out into a separate CI job (this is what I do personally) or to stick to older versions (or dropping the module all together)

Related PR:s and comments in this project:

Related PR:s and comments in other projects of mine:

I think that separating it out into its own module help for two reasons:

  1. It makes it so that less non-modern code is put directly into the project, making a project able to pretty much be 100% modern JS
  2. It can be tested across multiple real world projects (+ with tests in the project itself) to ensure it does the job its supposed to do. Something that is a bit out of scope of every individual project that uses it.

On top of version-guard I also use a reusable workflow of mine: https://github.com/voxpelli/ghatemplates/blob/main/.github/workflows/exit-silently-on-unsupported.yml

@voxpelli
Copy link
Member

Added a suggested solution in #1829

@theoludwig
Copy link
Member Author

The reason why its done like this is to allow easy upgrades to newer versions without it breaking projects that feel they need to support a wider set of Node.js versions than we do.

There is no reason for standard, dependency-check, installed-check or similar to fail when run on unsupported versions as that only forced those projects who have big test matrixes in their CI to either separate those projects out into a separate CI job (this is what I do personally) or to stick to older versions (or dropping the module all together)

Projects wanting to support a wider set of Node.js than standard is always possible!
As you said, those projects can separate CI jobs, also honestly linting should only run once, not for every Node.js supported version.
That's also why, I don't like to use the test npm scripts for "everything" CI, I think this is part of "best practices" to separate linting, unit tests, e2e tests, and other checks, each one in its own npm script, for example, npm run test only runs unit tests.
Then in CI, we run step by step, each npm script, we want.

As said in #1829 (review), I don't mind for the moment, if we still do "version guards" and fail silently, the priority now is to restore green CI and keep the same behavior as before, so we can close this issue.

But then once this issue is resolved, I think, it might be worth opening a new issue (with reference to this one), to discuss and debate the need for version guards, and actually "don't care", if run on a Node.js version that we don't support, basically, we are not responsible if it doesn't work and exit with status code 1.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants