-
Notifications
You must be signed in to change notification settings - Fork 1
Conversation
🦋 Changeset detectedLatest commit: 9679bb7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
46f0127
to
805709a
Compare
32121aa
to
a20362f
Compare
4d97f3b
to
32e3036
Compare
Size Change: -349 kB (-84%) 🏆 Total Size: 65 kB
|
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.
Don't know too much about Vite and its configuration but I also don't to gatekeep this PR!
As far as I understand this should affect how DCR uses discussion-rendering right? Just the bundling process for Storybook? With that in mind I'd quite like to see how it goes!
7c4d0b0
to
c3826bb
Compare
See the following issues: - storybookjs/storybook#18399 - storybookjs/storybook#15391
c3826bb
to
25b84b2
Compare
@@ -0,0 +1,3 @@ | |||
<script> | |||
window.global = window; |
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 does this do?
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 needed for some of the Storybook addons and the fix is suggested in storybookjs/storybook#18399 and https://github.com/storybookjs/builder-vite/blob/main/packages/builder-vite/README.md#migration-from-webpack--cra
@@ -0,0 +1 @@ | |||
/// <reference types="vite/client" /> |
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 does this mean?
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 enables IntelliSense for TypeScript
entry: "./src/App.tsx", | ||
formats: ["cjs","es"], | ||
fileName: (format) => { | ||
const path = format === "es" ? pkg.module : pkg.main |
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 not entirely sure I understand what's going on here? Could we add a comment, or could it be better for these values to be hardcoded?
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 ensures that the file name is coming from the package.json
, rather than having a loose contract between it and the build config.
Added a comment in 9679bb7.
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.
+1 A few questions to benefit my own understanding, but this looks good!
What does this change?
Use Vite for building everything:
yarn storybook
&yarn build-storybook
yarn dev
yarn build
Instead of having Webpack for Storybook and two Rollup configs for bundling the package, use Vite for everything.
The Rollup builds are performed in a similar time. The Storybook build takes about half the time on my machine, at about 35 seconds for
build-storybook
.Why?
Makes things faster and simpler, with a lot less tooling to maintain.
Hopefully this will help our CI tests to pass again, so we can start bumping dependencies. Dependabot seems to be unable to trigget chromatic, and trying to upgrade Source has been impossible with the current setup.
Helps move the needle on guardian/dotcom-rendering#6264