-
Notifications
You must be signed in to change notification settings - Fork 12
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
Do not inline the assets in the built package #37
Conversation
viteFinal: (config) => { | ||
return { | ||
...config, | ||
publicDir: "res", |
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.
There was no need to include those assets in the regular build, so I removed the publicDir option from the main vite.js config and put it here
"./dist/style.css": { | ||
"import": "./dist/style.css", | ||
"require": "./dist/style.css" | ||
"./dist/index.css": { |
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 is probably the only breaking change for consumers: this file is renamed style.css
-> index.css
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 looks pretty great! I had this on the backburner, very much appreciate you taking the time to look into it.
// We are *not* running in library mode because inlines all the assets. | ||
// Instead, we're running in normal mode & specifying the entrypoint here. |
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.
Let's add a link to the vite issue that tracks this, vitejs/vite#3295
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.
Done
// Must be in sync with the list of dependencies in the package.json | ||
// It includes all dependencies except @vector-im/compound-design-tokens | ||
external: [ | ||
"react", | ||
"react-dom", | ||
"react/jsx-runtime", | ||
"lodash", | ||
"classnames", | ||
"@radix-ui/react-form", | ||
], |
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.
Could we build that list dynamically by reading the package.json
and omitting @vector-im/compound-design-tokens
from that list.
I'm pretty certain we will fail to keep those two things in sync otherwise.
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 guess we could? We still would have special cases like react/jsx-runtime
, and I don't think this list would change a lot.
Worse thing which could happen is that the dependency ends up being included in the bundle, which is not that bad tbh 🤷
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.
No need to block the PR on this, i'll do it later on
Tracked as part of element-hq/compound#142
This opts out of vite's library mode, and ensure the built files look right.
4f60053
to
f10cb6c
Compare
Deploying with Cloudflare Pages
|
When I started using compound, I figured that the output CSS was over 1Mb. This was due to the assets being inlined.
This PR opts out of Vite's library mode. The downside is that we're not building a CommonJS-compatible bundle, and that we have to keep track of all external dependencies, but the upside is that the bundlers we use in the final apps should be a lot more intelligent about assets.
I also noticed that in the output, it was using a mix of
React.createElement
and the new_jsx
runtime. This was due to svgr handling the JSX transformation itself, so I made sure it was opting in the new JSX runtime.