front: use tsdown instead of rollup - #18201
Conversation
emersion
left a comment
emersion
left a comment
There was a problem hiding this comment.
running npm run build-ui time goes down from 7.7s to 4.9s on my machine
I'm relatively unimpressed with this speedup. I was hoping for sub-second build times, to be honest.
| "module": "dist/index.mjs", | ||
| "types": "dist/index.d.ts", | ||
| "main": "dist/index.esm.js", | ||
| "main": "dist/index.mjs", |
There was a problem hiding this comment.
Since we have "type": "module", why do we end up with the .mjs file extension?
See https://tsdown.dev/guide/how-it-works#smart-defaults
In general, exposing a .mjs file to outside users can be a pain because these aren't served with the text/javascript MIME type by Web servers.
There was a problem hiding this comment.
Agreed; it's because I did not specify that we were building a browser library (through the platform config), so the fixedExtension is true, so the smart default wasn't used. I'll change that.
Edit: It's done ✅
| transformer: 'postcss', | ||
| }, | ||
| deps: { | ||
| neverBundle: (id, parent, isResolved) => { |
There was a problem hiding this comment.
Do we really need this? Can't we set neverBundle: true?
See https://tsdown.dev/options/dependencies#externalizing-all-dependencies
There was a problem hiding this comment.
I prefered to err on the side of "not changing the behavior we had before", and this neverBundle function is very basically a port of what we had in the external configuration in rollup-base.
| "**/dist/**", | ||
| "**/storybook-static/**" | ||
| "**/storybook-static/**", | ||
| "**/tsdown.config.*" |
There was a problem hiding this comment.
Why ignore tsdown config files here?
There was a problem hiding this comment.
It was a way for me to get rid of some irrelevant warnings around the fact that they are the only files that run in a node environment (i.e. without the node types) so Typescript fired some errors.
In the mean time, I found a way to correctly wire Typescript so it doesn't complain anymore (with a tsconfig.node.json and a references: line on each tsconfig.json file), so let's do that instead.
Edit: It's done ✅
I agree, but I am convinced further improvement can be made my actually chaining those builds! (Edit: after discussing it with you IRL, we'll also have to check the workspace support using |
12ef96a to
6fcaa4c
Compare
github-actions
Bot
added
the
area:ci
|
Rebased on I also updated |
gaetan-osrd
left a comment
gaetan-osrd
left a comment
There was a problem hiding this comment.
LGTM for the dependabot changes
32698c2 to
44f579e
Compare
ac92550 to
634fa4b
Compare
tsdown is somehow a little bit more strict around the usage of the "type" keyword on exports. Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
To do so, we simply have to be explicit on inline paths; using relative, we make them relative to the main.css file (not the one where we use "inline"). Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
Most of the changes in the new svgToReact.ts file is a translation to a Typescript file (with a few types added there and there) and wrapping everything into a function. Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
Signed-off-by: Stanislas Signoud (Signez) <signez@stanisoft.net>
634fa4b to
4d232ef
Compare
3 participants
Did you remember #15693? Yeah, maybe it's time we switch from
rollupto the more moderntsdown!We can't use
rolldowndirectly because it doesn't support CSS bundling at all. It's not a bad thing though, because it pushed me to investigate what is the state of the art in library bundling, and it happens to betsdown, that we could describe as "vite, but for libraries".It works well: running
npm run build-uitime goes down from 7.7s to 4.9s on my machine, and it also offer the cool feature of actually forwarding Ctrl+click declarations between dependencies (through the usage of thedts.sourcemapfeature). And it also properly cleans up thedistfolder, so we don't have stale files problems.Note that I didn't keep the "base" config, as I think the new config files are simple enough it doesn't provide that much of an upside to keep this indirection, in my humble opinion.