Simplify our building packages tool (drop Rollup for tsdown) #2935
Conversation
📊 Packages dist files size differenceThanks for the PR! Here is the difference in size of the packages dist files between the base branch and the PR.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Converting in draft, I will investigate why Turbo E2E tests do not pass anymore EDIT: fixed when upgrading the target from |
… fix its inlining in final `live_controller.js`
| @@ -17,7 +17,7 @@ | |||
| "main": "dist/live_controller.js", | |||
| "types": "dist/live_controller.d.ts", | |||
| "config": { | |||
| "css_source": "styles/live.css" | |||
| "css_source": "src/live.css" | |||
There was a problem hiding this comment.
I moved the live.css file under src aside js source code, otherwise tsdown/rolldown create the following files:
dist/src/live_controller.jsdist/styles/live.min.css
Which is not what we want.
Note: other UX packages define their styles in src/ too
That’s actually what worries me the most… what happens to users currently importing types directly from those files? For all the rest I’m very 👍 PS: let’s be very very attentive to details here and consider all potential outcomes — we now have a non-negligible number of users and, as you mentioned, we still have a BC promise to keep 😅 |
|
How to explain the difference in LiveComponent dist/controller.js ? idiomorph ? |
| ## 2.28.0 | ||
|
|
||
| - [BC BREAK] If you are using the Symfony AssetMapper but **not** Symfony Flex, | ||
| you need to upgrade your `importmap.php` and change the asset `react-dom` to `react-dom/client`, | ||
| and run `php bin/console importmap:install`. | ||
|
|
||
| Symfony Flex or Webpack Encore users are not affected. |
There was a problem hiding this comment.
I really don't see why we would release this. No feature, no bug, but a big break in userland code.
Let's at least wait to push until we have something to....
There was a problem hiding this comment.
I understand your point which is totally legitimate, but the current behavior with Rollup and Rollup TypeScript plugin is buggy.
When building the package, our import { createRoot } from 'react-dom/client'; is modified to this weird import require$$0 from 'react-dom'; import, that use react-dom instead of react-dom/client.
This behaviour was fixed by rollup/plugins#1310, but then I had to find a hack to re-introduce the bug in order to prevent a BC, see https://github.com/symfony/ux/blob/2.x/bin/rollup.ts#L125-L129
I may be biased, but who does not use Flex in 2025? 😛
More generally, it's a bit unlucky to have to restrict ourselves here because of AssetMapper/Importmap, we lose the freedom to change some pure JavaScript related things where it wouldn't be a problem with in a full Node.js ecosystem (with npm, webpack Encore, ...) 😕
I will try to investigate if we can re-add this import require$$0 from 'react-dom'; (or equivalent), but I don't think this will be possible
There was a problem hiding this comment.
Ok I was not yet here when i commented previous comment :)
Thanks for the detailed explanation... I still think that releasing a version that will break people apps, when we bring zero new feature, is neither a good habit nor a good message to send 🤷
Yeah I agree with you
But i understand you want to change everything at once... So to me we need to find a way to "not" release React here.
WDYT ?
I understand, but I don't think that's good to maintain two similar but different building system.
... What about releasing UX 3 soon, with this PR and issues from https://github.com/symfony/ux/issues?q=is%3Aissue%20state%3Aopen%20milestone%3A3.x fixed too?
There was a problem hiding this comment.
I agree with you on everything... i just cannot stress enough that a change in build is not a feature.
And maintain npm only package is not something i refuse or don't want (they would still work with importmap :) )...
... i really think without a way to synchronize releases for both php and js vendor... and this is almost impossible to do properly 🤷
| @@ -8,7 +8,8 @@ | |||
| "rootDir": "src", | |||
| "strict": true, | |||
| "strictPropertyInitialization": false, | |||
| "target": "es2021", | |||
| "//": "The target should be kept in sync with `bin/build_packages.ts` file.", | |||
| "target": "es2022", | |||
There was a problem hiding this comment.
Can we add polyfills for Object.hasOwn() and Array.at() ?
There was a problem hiding this comment.
Array.at() is available at 93.9% and Object.hasOwn() at 93.87%, sure?
There was a problem hiding this comment.
Ok yeah that seems fair (even 97% for both in real usage it seems) ..
.. but after this one, please can we say we do not make changes like this in minor/patch version ? 😆 🙏
|
Let's wait a bit until tsdown and rolldown become more mature. |
|
Opened #2940 with some fixes added here. |
This PR was squashed before being merged into the 2.x branch. Discussion ---------- [Icons] Some fixes on LiveComponent assets | Q | A | ------------- | --- | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Docs? | no <!-- required for new features --> | Issues | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT <!-- Replace this notice by a description of your feature/bugfix. This will help reviewers and should be a good start for the documentation. Additionally (see https://symfony.com/releases): - Always add tests and ensure they pass. - For new features, provide some code snippets to help understand usage. - Features and deprecations must be submitted against branch main. - Update/add documentation as required (we can help!) - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry - Never break backward compatibility (see https://symfony.com/bc). --> Fixing some incoherent things found in #2935, each commit are separated and contains a link to the comment explaing why Commits ------- c3be957 [Icons] Make `idiomorph` as a devDependency, since it's inlined in dist files (#2935 (comment)) a5bae94 [Icons] Move `assets/styles/live.css` to `assets/src/`, like other packages (#2935 (comment))
…ce Rollup by tsup (Kocal) This PR was squashed before being merged into the 2.x branch. Discussion ---------- Modernize and simplify our packages building tools, replace Rollup by tsup | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes <!-- please update src/**/CHANGELOG.md files --> | Docs? | yes <!-- required for new features --> | Issues | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT Following #2935. Same goals, but less frictions than with tsdown, [tsup](https://github.com/egoist/tsup): - `target` is correctly read from our `tsconfig.packages.json` - no `//#region` comments - less polyfills than oxc - the `target` **stays** `es2021` without any issues with `static` and Stimulus Two things: 1. About the `react-dom/client` import, yes it will impact AssetMapper users that don't use Flex, but it will positively impact other users (AssetMapper with Flex, Webpack Encore...) by removing useless code and making the file smaller 2. About the tons of `.d.ts` files removed, that's still fine, there is no point to generate `dist/<file>.d.ts` files when `dist/<file>.js` do not exist, they are not part of the public API. The code review must be easier, since less code has been touched than with tsdown. --- # Demo When building LiveComponent assets: ``` ➜ assets git:(tsup) pnpm build > `@symfony`/[email protected] build /Users/kocal/workspace-os/symfony-ux/src/LiveComponent/assets > tsx ../../../bin/build_package.ts . CLI Building entry: src/live.css, src/live_controller.ts CLI Using tsconfig: ../../../tsconfig.packages.json CLI tsup v8.5.0 CLI Target: es2021 CLI Cleaning output folder ESM Build start [Symfony UX] Minified CSS file: /Users/kocal/workspace-os/symfony-ux/src/LiveComponent/assets/dist/live.css [Symfony UX] Renamed dist/live.css to dist/live.min.css ESM dist/live_controller.js 12.25 KB ESM dist/live.css 74.00 B ESM ⚡️ Build success in 26ms DTS Build start DTS ⚡️ Build success in 1276ms DTS dist/live_controller.d.ts 7.96 KB ``` When watching LiveComponent assets, the CSS is easily watched too! https://github.com/user-attachments/assets/a246f278-8bf4-40f6-887e-685be7509af0 Commits ------- 78fa229 Rebuild packages with tsup 897aaa8 Modernize and simplify our packages building tools, replace Rollup by tsup
…lace Rollup by tsup (Kocal) This PR was squashed before being merged into the 2.x branch. Discussion ---------- Modernize and simplify our packages building tools, replace Rollup by tsup | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes <!-- please update src/**/CHANGELOG.md files --> | Docs? | yes <!-- required for new features --> | Issues | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead --> | License | MIT Following #2935. Same goals, but less frictions than with tsdown, [tsup](https://github.com/egoist/tsup): - `target` is correctly read from our `tsconfig.packages.json` - no `//#region` comments - less polyfills than oxc - the `target` **stays** `es2021` without any issues with `static` and Stimulus Two things: 1. About the `react-dom/client` import, yes it will impact AssetMapper users that don't use Flex, but it will positively impact other users (AssetMapper with Flex, Webpack Encore...) by removing useless code and making the file smaller 2. About the tons of `.d.ts` files removed, that's still fine, there is no point to generate `dist/<file>.d.ts` files when `dist/<file>.js` do not exist, they are not part of the public API. The code review must be easier, since less code has been touched than with tsdown. --- # Demo When building LiveComponent assets: ``` ➜ assets git:(tsup) pnpm build > `@symfony`/[email protected] build /Users/kocal/workspace-os/symfony-ux/src/LiveComponent/assets > tsx ../../../bin/build_package.ts . CLI Building entry: src/live.css, src/live_controller.ts CLI Using tsconfig: ../../../tsconfig.packages.json CLI tsup v8.5.0 CLI Target: es2021 CLI Cleaning output folder ESM Build start [Symfony UX] Minified CSS file: /Users/kocal/workspace-os/symfony-ux/src/LiveComponent/assets/dist/live.css [Symfony UX] Renamed dist/live.css to dist/live.min.css ESM dist/live_controller.js 12.25 KB ESM dist/live.css 74.00 B ESM ⚡️ Build success in 26ms DTS Build start DTS ⚡️ Build success in 1276ms DTS dist/live_controller.d.ts 7.96 KB ``` When watching LiveComponent assets, the CSS is easily watched too! https://github.com/user-attachments/assets/a246f278-8bf4-40f6-887e-685be7509af0 Commits ------- 965fb95 Modernize and simplify our packages building tools, replace Rollup by tsup
This PR simplify a lot our building process by moving from Rollup to tsdown, based on Rolldown (compatible with Rollup API).
Why?
Rolldown is much modern and faster than Rollup (note: we do not really see the speed impact here, since our files are pretty small).
Tsdown is a library bundler based on Rolldown, and it requires far less configuration or manual tweaks than our actual solution:
The file
bin/build_package.tsbecomes smaller and easier to understand, and we don't even needbin/rollup.tsfile anymore!Review
When reviewing the PR, you will notices some modifications on
dist/files:the code contains
//#region: these are regions useful when reading the file in an IDE like VSCode, it allows to fold/unfold specific portions of the code. Unfortunately it's not configurable, but we can remove them by minifying them (thanks tominifyoption)I feel like the code contains more polyfill and other helpers than before: this is because rolldown use the oxc toolchain which generates a similar but different code than Rollup/TypeScript.
This is fine for the moment, but in a next PRI will upgrade the target toes2022or something similar (statickeyword and private class fields/methods are now supported at 95%)EDIT: the current polyfill for
statickeyword with targetes2021break Stimulus controllers, upgrading toes2022works fineA lot of files are removed: that's fine, it's only about
.d.tsfiles. Their types can now be found in the main dist file (e.g.controller.d.ts)Demo
When building a single package:
When watching a single package:
Enregistrement.de.l.ecran.2025-07-21.a.23.38.16.mov