Conversation
|
Size Change: 0 B Total Size: 532 kB ℹ️ View Unchanged
|
check the following places: - .nvmrc - riff-raff.yaml - Containerfile
mxdvl
force-pushed
the
mxdvl/node-versions
branch
from
e020058 to
6383812
Compare
Member
|
would this work better as a commit hook that checks all those files if any of them appear in the list of changed files? |
Contributor
Author
What’s the benefit of a commit hook over a CI check? Is it only about how quickly the developer is informed of the error? Is there any risk that it could be disabled in certain contexts? |
Member
yes, basically. if we're going to check something, why wait until last possible moment to check it?
not really, you'd have to work quite hard to get round husky while still finding a way to install NPM packages. that said, would it work better as part of |
Contributor
Author
|
I appreciate the suggestions, but the main issue is that would make Deno a hard requirement for contributors to commit on this codebase, which is not currently the case. Considering that at the current cadence LTS versions of Node are released at most once a year in late October, this proposal attempts to catch a bug which can only happen on very rare occasions. Ideally, I would leave further tweaks and refinements for @guardian/dotcom-platform to implement. |
Member
if the idea is to catch a mistake, and mistakes are best caught as early as possible, then is deno the right technology to use here? our choice of technology should be guided by our requirements – this could be written in Node or bash without adding any new dependencies. i reckon we could also get away with a regex check in all 3 files, that should be enough? that way we can remove the dependencies of the current deno script too |
Contributor
Author
|
I implemented this quickly as a reaction to a chat thread that asked whether it would be technically feasible, but have little time to implement a different approach. If this PR is deemed unsatisfactory for any reason I’ll simply close it and let the team figure out a solution by end of April 2025.
My aim here was to maximise safety by using a strongly-typed and fully parsed approach. The issues with alternatives are as follows:
|
Contributor
Author
|
Closing in favour of an incoming pre-commit hook using a |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this change?
Compare node versions in
.nvmrc,riff-raff.yamlandContainerfileto ensure that they are aligned.Why?
Helps with updates such as #7948
Left as an exercise to the reader
Implement a check for
@types/node😉