-
Notifications
You must be signed in to change notification settings - Fork 916
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
Move install logic into separate package #1042
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pikapkg/snowpack/kgmv6dt6z |
I’d be curious how much effort it’d be to just rename |
|
df726f8
to
384d9a6
Compare
384d9a6
to
6d2106a
Compare
6d2106a
to
54d3342
Compare
54d3342
to
d487834
Compare
452768c
to
63d5a9a
Compare
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 just looked through the code changes, I didn't test the changes in detail. Let me know if you want me to have a thorough look
}, | ||
"scripts": { | ||
"build": "pika build", | ||
"version": "pika build" |
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.
that doesn't look right?
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 this makes sense; it’s a hook that happens before publishing to npm making sure a fresh build is prepared (i.e. it’s not stale, from a previous commit or different branch)
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 "version" script is a hook that happens before publishing? Are you sure?
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.
Not technically it doesn’t, but in our workflow since we use Lerna, the versioning happens before publishing (I think? Please correct me if I’m still mistaken!).
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.
Looks good! I didn’t compare how much esmpkg deviated from the original install script, but nothing seemed to jump out. Seems straightforward, and the updated tests give me confidence it’s working the same. So excited for this!!! 💯 💯
63d5a9a
to
ac6e736
Compare
Resolves #312
Background - Community
Background - Snowpack
New Package:
esmpkg
Today, this is just the idea of moving Snowpack's "install" command into a new package. This means no change to how Snowpack works, but a few benefits right off the bat:
In the future, esmpkg can lazer-focus on improvements to support better CJS->ESM conversions and begin working on conversions for Node.js (in addition to our current web support).
Testing