-
Notifications
You must be signed in to change notification settings - Fork 3k
feat: add npm init support of git, path, url, and version names #20403
Conversation
zkat
left a comment
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 for taking the time to keep improving this feature, @jdalton! I've made a first pass at this PR.
lib/init.js
Outdated
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.
This line is kinda confusing, tbh. I'm not really sure at first glance how it might be different than the previous one (since it's not clear this is about non-registry packages).
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 think I saw <pkg> used in another place to mean <folder|tarball|git url|user/repo>. Maybe a way to say everything else is just forwarded to npx.
lib/init.js
Outdated
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 wonder if we could do more here. Should we also, maybe, handle hosted git deps? npm init zkat/templ -> npx github:zkat/create-templ? What's the expectation around this? What makes these others any better than doing npx file:some/initializer?
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.
What makes these others any better than doing npx file:some/initializer?
At the moment they aren't any better than using npx. We could keep it simple. When a user provides a non registry name to npm init it could just recommend using npx instead. Something like
$ npm init file:some/initializer
> Unrecognized initializer: file:some/initializer.
> For more package binary executing power check out `npx`:
> https://www.npmjs.com/package/npxThere 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.
Ok, I've updated the PR to more gracefully punt on git, path, and url names for now instead of just erring out.
lib/init.js
Outdated
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.
This should probably be an actual error. Maybe with err.code === 'EUNSUPPORTED' or something? 🤔
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.
Ah ok!
|
Made the |
zkat
left a comment
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'm v happy with this. Thanks, @jdalton!
|
Thank you! |
This PR addresses #20399.