-
Notifications
You must be signed in to change notification settings - Fork 187
feat: add vite to bundle #740
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
3aab1cd
to
35d6fa4
Compare
35d6fa4
to
d4f3e1d
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.
Overall this looks really tidy 💅🏼
I found only one thing that I'm not sure of: dist/index.cjs
contains only require
statements and not the whole bundle. Is that intentional? Would e.g. webpack
resolve these dependencies even if we do not transpile node_modules
and consume cjs files? I think in the ESM case it does, but I'm not sure about cjs.
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 good with these changes. Thanks again @joshuaellis for pushing it forward 🚀
I think in terms of merge/ release I'd prefer if we'd release 1.26
asap followed by a 1.3
alpha/ beta release, so that both (cloud and cms) can pull it in to see if that works well.
What do you think?
I agree, I'll try to get 1.2.6 released today as there's bits for |
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.
Good job @joshuaellis !
What does it do?
This PR does a couple things:
Ideally, I'd like to lift out storybook into it's own
docs
folder and make that isolated from the packages so we have toimport { Select } from '@strapi/design-system'
so thatdesign-system
package and keep it tidier in the long run.This would require moving all the stories around and I want to get the opinion on it before actioning.
Why is it needed?
Because moving to
vite
decreases bundle size by ~70-80% and therefore allows better tree shaking in other applications.Keeping the packages tidy as possible to ensure we're not adding additional overheads where not required i.e. when people install our package imo they shouldn't be installing all the unneccessary devDeps for our storybook etc.
Notes
Because of the changes totype:module
in the package.json, #741 needs to be merged first.It also might be worth releasing a
beta
of this prior to stable to ensure it correctly works withcloud
andcms
.Therefore, we could set up a
beta
branch – thoughts?