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

lint-staged requires at least version 8.12.0 of Node #233

Closed
artem-zakharchenko opened this issue Jul 8, 2019 · 9 comments · Fixed by #221
Closed

lint-staged requires at least version 8.12.0 of Node #233

artem-zakharchenko opened this issue Jul 8, 2019 · 9 comments · Fixed by #221
Labels

Comments

@artem-zakharchenko
Copy link
Contributor

When performing a commit, a pre-commit hook from Husky executes the lint-staged command, which produces the following error:

husky > pre-commit (node v8.11.4)
lint-staged requires at least version 8.12.0 of Node, please upgrade
husky > pre-commit hook failed (add --no-verify to bypass)

This behavior is reproducible on both 9.0.1 and 9.0.2 versions of lint-staged.

Perhaps, we should consider setting the NodeJS version to at least 8.12.0, or revert to the version of lint-staged that does not have such limitation.

@honzajavorek
Copy link
Contributor

Wow, limiting Node versions to minor numbers? That's quite unexpected, I'd assume tools would be more benevolent in this. Is there a good reason why they do so?

I'd be hesitant to allow only for certain minor versions of Node 8.x.x, that would need to propagate to Dredd and other things as well. Also we support anything with 8 as a major now, so it would be a breaking change. Perhaps we could specify that even though Gavel works with 8 and is tested with 8, for development 8.12.0 is needed, but that's just counter-intuitive.

@honzajavorek
Copy link
Contributor

honzajavorek commented Jul 8, 2019

This is the PR where they introduced it lint-staged/lint-staged#646. The 💩 falls down from sindresorhus/execa#319.

@artem-zakharchenko Is the message produced by lint-staged really an error or is it a warning? I think the reason why the node version is required isn't so relevant to what Gavel does nor to what lint-staged does, they just blindly require the same as execa. If it was a warning we can live with it. If it is an error, we should probably mention in README that 8.12.0 is required for development and let it be ¯\_(ツ)_/¯

I think there's no reason to pin Gavel's requirement to 8.12.0 as Gavel itself runs on any Node 8. And contributors will probably opt for Node 10 or 12 anyway, what do you think?

@honzajavorek
Copy link
Contributor

Also I don't know if I want to pin lint-staged as we'd have to remember to re-enable dependabot again on it once we drop Node 8 and we'd lose security fixes.

@artem-zakharchenko
Copy link
Contributor Author

The message produced terminates the process, forbidding to perform a commit (terminates a husky process for pre-commit hook). Therefore, it's something we should address on our side.

One solution would be to downgrade to lint-staged@9.0.0, as mentioned in the attached thread, that does resolve the issue. Another would be to check if husky is up-to-date and can fix the issue for us.

I am against changing the version of Node required by Gavel, as it alters the meaning of the required version (the one needed to operate, not to develop).

@honzajavorek
Copy link
Contributor

What do you think about recommending to develop Gavel with Node 10?

@artem-zakharchenko
Copy link
Contributor Author

If we lock the Node version with .nvmrc and mention the usage of nvm somewhere in the contribution guidelines, then it should be fine.

@honzajavorek
Copy link
Contributor

I see nvm as just one of many ways how to install Node, but why not. Note in README and .nvmrc sound okay.

@artem-zakharchenko
Copy link
Contributor Author

Addressed in 73b7a95 as it blocks the work on the pull request.

@ApiaryBot
Copy link
Collaborator

🎉 This issue has been resolved in version 7.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

3 participants