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

Support npm >= 7 through RFC'd config section #105

Merged
merged 1 commit into from
Apr 6, 2022

Conversation

danielgindi
Copy link
Contributor

This supports both the old way in npm 6, and npm >= 7 through the new RFC'd config section.

@searls searls requested a review from jasonkarns March 21, 2022 11:55
@searls
Copy link
Member

searls commented Mar 21, 2022

Thanks!

@rosston
Copy link
Member

rosston commented Mar 28, 2022

I've confirmed this works for me in the example project with both npm v6 and v8.

Here's what I did:

  1. Checked out the code in this PR
  2. Ran npm i in the project root
  3. Use node 14, and the default npm 6
  4. In the example project, add "scripty": {"dryRun": true} to the package.json
  5. Confirm that npm t does a dry run
  6. Change the example/package.json to use "config": {"scripty": {"dryRun": true}} instead
  7. Confirm that npm t still does a dry run
  8. Use node 16, and the default npm 8
  9. Confirm that npm t still does a dry run

One thing I noticed however is that npm t in the root project is failing on this PR, regardless of npm version. I haven't dug into that at all yet. We should probably get this project back on CI (travis-ci.org doesn't build anymore), and then that test failure would be more obvious. 🙂

@rosston
Copy link
Member

rosston commented Mar 28, 2022

And just for my reference later: I'm pretty sure moving this PR forward/merging it will resolve #93 and #103.

@jasonkarns
Copy link
Member

Do we need to add a test case to cover an npm_package_config_scripty_X env var still plays well with scripty's own env var parsing?

Does the npm lifecycle env var still exist? That's the only other one outside of config that I think we rely on.

Should we also document that npm_package_config_scripty_* work or no?

@danielgindi
Copy link
Contributor Author

Does the npm lifecycle env var still exist? That's the only other one outside of config that I think we rely on.

Yes. It's in the RFC, and validated out there in real life 😉

Should we also document that npm_package_config_scripty_* work or no?

Should be obvious the amount as it was before without config... The full env behavior is in the rfc, which is the major breaking change in npm 7.

@danielgindi
Copy link
Contributor Author

danielgindi commented Apr 6, 2022

Are we merging this? (Does not even require a major version release)

@searls searls merged commit b736870 into testdouble:main Apr 6, 2022
@searls
Copy link
Member

searls commented Apr 6, 2022

Thanks! Merged and released as 2.1.0

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.

4 participants