-
-
Notifications
You must be signed in to change notification settings - Fork 4
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
Update dependencies, apply fixes from "upstream" #7
Conversation
* Update all npm dependencies to resolve install warnings regarding deprecated modules. * Add an 'engines' declaration to package.json since the current version of 'glob' requires Node 20 or 22 * Change the package to ESM since dependency 'get-port' is ESM and can't be require'd anymore [1] [1]: https://github.com/sindresorhus/get-port/releases/tag/v6.0.0
…g brew Basically applying [1] and following changes made in davewasmer/devcert [1]: davewasmer@a1815d7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good to me, happy to post a release.
package.json
Outdated
"scripts": { | ||
"prepublish": "tsc" | ||
}, | ||
"repository": { | ||
"type": "git", | ||
"url": "git+https://github.com/guybedford/devcert.git" | ||
}, | ||
"engines": { | ||
"node": "^20.11.0 || >=22.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node.js 14+ support ES modules, so in theory this should work there I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current version of glob requires Node 20, so I thought it would be reasonable to align the minimum versions with that.
I chose 20.11 instead of 20.0 since it provides almost the same APIs as 22.0. Node 21 is already out of maintenance.
But no problem to change this to 14+ if you think that would be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we should not be updating to this version of glob!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. The same applies to the latest version of rimraf but I don't see a problem there either.
get-port v7 requires Node 16, should we downgrade to v6 as well? I don't see relevant fixes/features in the current v7 releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I downgraded all three dependencies and changed the engines
field to what seems reasonable based on what the dependencies state.
I have to add that I didn't test this with an older Node.js version than 20, since the project we use this package in, does not support those anymore 😅
Awesome, thanks! |
…de execution Adapted from davewasmer@e0e8ae3
16ec6e8
to
46e5f4e
Compare
I've published a 0.5.0 here. |
@guybedford thanks a lot for releasing a new version. Unfortunately, the release does not contain the actual code https://www.npmjs.com/package/devcert-sanscache?activeTab=code. The whole lib folder is not present at all. I guess |
@flovogt thanks I've posted a 0.5.1. |
This PR was mainly targeted at updating npm dependencies in order to resolve deprecation warnings produced by "npm install" right now:
❯ npm i npm warn deprecated inflight@1.0.6: This module is not supported, and leaks memory. Do not use it. Check out lru-cache if you want a good and tested way to coalesce async requests by a key value, which is much more comprehensive and powerful. npm warn deprecated rimraf@2.7.1: Rimraf versions prior to v4 are no longer supported npm warn deprecated glob@7.2.3: Glob versions prior to v9 are no longer supported
One of the dependencies switched to ESM, so I had to do the same for this package in order to use imports. Hope that is fine with you. This is a breaking change for consumers that are still using CommonJS.
While checking the origin project "davewasmer/devcert" I noticed two changes that I found reasonable to apply to this fork, see below.
I also included the 0.4.8 commit in this PR since it is missing on master but the tag is there.
I only tested these changes on macOS. Thank you for taking your time to look into this!
First Commit
refactor: Update dependencies and switch to ESM
deprecated modules.
version of 'glob' requires Node 20 or 22
be require'd anymore: https://github.com/sindresorhus/get-port/releases/tag/v6.0.0
Second Commit
fix(darwin): Properly detect whether nss has been installed when using brew
Basically applying davewasmer@a1815d7 and following changes made in davewasmer/devcert
I ran into this while testing on my machine.
Third Commit
fix: Switch from execSync to execFileSync to reduce risk of remote code execution
Adapted from davewasmer@e0e8ae3