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

Print a warning when used with Node versions other than v16.15.0 #4366

Merged
merged 3 commits into from Jun 21, 2023

Conversation

p2edwards
Copy link
Contributor

@p2edwards p2edwards commented Mar 17, 2023

Description

Warn in development when running the JavaScript frontend with unsupported Node or NPM version.

Summary

Issue a warning if not running with npm 8.5.5, since the errors that you get if you run a different version are not very obvious.

  • Help contributors who switched their Node version inadvertently.
  • Support new contributors by printing more context / steps to remedy.
  • Give the benefit of the doubt to developers who run a different version on purpose. So, don't process.exit(1), even on npm install.

Show on preinstall. Since it's easy to miss there, also show it on other run scripts such as 'watch'.

Screenshot Npm Version Warning

Notes for reviewer

Decisions that came up:

  • Should we run process.exit(1) if npm install is attempted with an unsupported npm version?
    • Decision: No, just print the warning. Give the developer the benefit of the doubt.
  • When should we run the warning?
    • Decision: On npm install, as a preinstall step. Yet since that message doesn't print immediately, and may get buried in the various messages npm install prints out, also run it on build, watch, test, and test-autobuild.
    • Alternatives: Could run it on postinstall after copy-fonts, making it more prominent. It's still a bit buried though.
  • Should the output be colorized?
    • Decision: Yes, to make it easy to see.
    • Implementation: For simplicity, I didn't use any external libraries, nor try to detect terminal color support. I figured most CI or containerized environments either won't encounter this message or will have proper color support.
  • Should there be an environment variable to disable this check/warning?
    • Decision: No, that seems counterproductive.
  • Should we explain/recommend a Node version manager in this warning?
    • Decision: No, to save space.
    • Alternatives: Could link to a page in the developer documentation, or a GitHub issue.
    • For example, it might be helpful to mention something like eval "$(fnm env --use-on-cd)", which reads .node-version and switches to v16.15.0 whenever you enter the kpi folder in the terminal.

Checklist

  1. If you've added code that should be tested, add tests
  2. If you've changed APIs, update (or create!) the documentation
  3. Ensure the tests pass
  4. Make sure that your code lints and that you've followed our coding style
  5. Write a description of your work suitable for publishing on our forum
  6. Mention any related issues in this repository (as #ISSUE) and in other repositories (as kobotoolbox/other#ISSUE)
  7. Open an issue in the docs if there are UI/UX changes

Related issues

Related to #3851

Issue a warning if not running with npm 8.5.5, since the errors
that you get if you run a different version are not very obvious.

- Help contributors who switched their Node version inadvertently.
- Support new contributors by printing more context / steps to remedy.
- Give the benefit of the doubt to developers who run a different
  version on purpose. So, don't process.exit(1), even on npm install.

Show on preinstall. Since it's easy to miss there, also show it on
other run scripts such as 'watch'.
@p2edwards p2edwards self-assigned this Mar 17, 2023
@p2edwards p2edwards requested a review from jnm March 17, 2023 21:55
I sure thought I'd fixed this already.
Base automatically changed from faster-frontend-builds to beta March 21, 2023 23:34
Copy link
Member

@jnm jnm left a comment

Choose a reason for hiding this comment

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

Fixed merge conflict; waiting for CI to complete

@jnm jnm merged commit 15a5209 into beta Jun 21, 2023
3 of 4 checks passed
@jnm jnm deleted the warn-for-npm-version branch June 21, 2023 21:25
@p2edwards p2edwards mentioned this pull request Jul 5, 2023
1 task
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.

None yet

2 participants