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

fix: warn message on engine mismatch #257

Closed

Conversation

ruyadorno
Copy link
Contributor

@ruyadorno ruyadorno commented Sep 26, 2019

Overview

Messages for mismatching engine fields are not printed to users currently, contrary to the documented behavior, this PR fixes it by making sure that warning messages from packages containing mismatch engines are actually printed at WARN level for users of the cli.

✏️ Changes

  • Fixed setting engine check warnings to each package
  • Added proper catching of package warnings during validation phase
  • Added tap test that validates print of engine warn msgs

🔗 References

📸 Screenshots

Before

Screen Shot 2019-09-26 at 11 23 53 AM

After

Screen Shot 2019-09-26 at 11 24 11 AM

🔍 Testing

Manual testing

  1. Setup a package with some invalid engines.node value, eg:
{
  "name": "engine-module",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "engines": {
    "node": ">=100"
  }
}
  1. Try to install from it, eg: npm install ./engine-module
  2. Should be able to observer WARN messages of type notsup

Automated testing

  • unit test at test/tap/validate-tree-check-warnings.js checking that package warnings are correctly collected to top level tree
  • integration test at test/tap/check-engine-reqs.js validating that warning messages are printed to users upon npm install

✅ This change has unit test coverage
✅ This change has integration test coverage

🔥 Rollback

If we observe something wrong with this change in production, how should we respond?

This can be reverted at any time

- Fixed setting engine check warnings to each package
- Added proper catching of package warnings during validation phase
- Added tap test that validates print of engine warn msgs

Fix: https://npm.community/t/engines-and-engines-strict-ignored/4792
@ruyadorno ruyadorno requested a review from a team as a code owner September 26, 2019 16:37
@ruyadorno ruyadorno added the semver:patch semver patch level for changes label Sep 27, 2019
@ruyadorno
Copy link
Contributor Author

@npm/cli-team pushed a new implementation that uses the top tree instead:

  • pros: less disruptive, needs less lines of code and it’s closer to the original intent there
  • cons: had to change the signature of the isInstallable method from lib/install/validate-args.js in order to pass in a tree reference
  • cons: one of the place that consumes isInstallable doesn’t seem to have a tree reference (lib/install/actions.js) but I made sure the implementation is resilient to that (will just ignore warnings as it did before)

those are the cons that made me steer away from this implementation originally but seeing the unexpected behavior (such as saving metadata) from using the manifest package I’m seeing this as a safer option now 😊 would be nice to have your insight

isaacs pushed a commit that referenced this pull request Oct 8, 2019
@isaacs isaacs mentioned this pull request Oct 8, 2019
@isaacs isaacs closed this in 5aba50a Oct 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch semver patch level for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant