-
Notifications
You must be signed in to change notification settings - Fork 246
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
fix(clerk-js): Bundle CSS into CJS and ESM bundles #4301
Conversation
|
const clerkUICSSLoader = () => { | ||
// This emits a module exporting the URL to the styles.css file. | ||
// This emits a module exporting a URL to the styles.css file. |
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.
These could be one loader with a boolean argument for supportsChunks
. I didn't have a strong opinion one way to the other, so happy to change this if anyone else has a stronger opinion!
* Used for production builds that combine all files into one single file (such as for Chrome Extensions). | ||
* @type { () => (import('webpack').Configuration) } | ||
* */ | ||
const commonForProdBundled = () => { |
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.
We could also do something like commonForProd({ chunked: true })
and do everything within the commonForProd
function.
@@ -283,13 +326,19 @@ const prodConfig = ({ mode }) => { | |||
], | |||
}); | |||
|
|||
const clerkCjs = merge(clerkEsm, { | |||
const clerkCjs = merge(entryForVariant(variants.clerk), common({ mode }), commonForProd(), commonForProdBundled(), { |
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 removed the merge(clerkEsm
portion because the experimental ESM output was confusing Webpack. It was emitting references to import.meta.url
within the CJS output, which caused ATTW to complain.
Description
This PR updates the CJS and ESM builds of
clerk-js
to bundle imported CSS files instead of emitting a URL to the file. This is required since we don't have the capability to dynamically load anything on account of not knowing the public path (like we do in our browser bundles). This impacts consumers who directly consume theclerk.js
andclerk.mjs
bundles ofclerk-js
, which includes Chrome extensions (and possibly others).Checklist
npm test
runs as expected.npm run build
runs as expected.Type of change