-
-
Notifications
You must be signed in to change notification settings - Fork 372
Modernize and simplify our packages building tools, replace Rollup by tsup #2944
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
Conversation
📊 Packages dist files size differenceℹ️ No difference in dist packagesFiles. |
a935902
to
5e580c5
Compare
"types": ["node", "@testing-library/jest-dom"], | ||
"lib": ["es2022", "dom"] |
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.
Not really related to this PR, but it helped to get correct types and syntax highlighting when working on bin/build_package.ts
file
@@ -7,4 +7,168 @@ | |||
* file that was distributed with this source code. | |||
*/ | |||
|
|||
export * from './translator'; |
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.
Conflicted with options.bundle
from ESBuild, but in reality it didn't bring any value to externalize the translator logic if the final goal is to export everything in the controller. Just define everything in the controller, that's simpler.
import { | ||
eagerControllers, | ||
isApplicationDebug, | ||
lazyControllers | ||
} from "./controllers.js"; |
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.
Thanks to options.bundle = false
from ESBuild and external
from tsup, the file controllers.js
is not inlined, no BC 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.
Nice
function requireClient () { | ||
if (hasRequiredClient) return client; | ||
hasRequiredClient = 1; | ||
|
||
var m = require$$0; | ||
if (process.env.NODE_ENV === 'production') { | ||
client.createRoot = m.createRoot; | ||
client.hydrateRoot = m.hydrateRoot; | ||
} else { | ||
var i = m.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED; | ||
client.createRoot = function(c, o) { | ||
i.usingClientEntryPoint = true; | ||
try { | ||
return m.createRoot(c, o); | ||
} finally { | ||
i.usingClientEntryPoint = false; | ||
} | ||
}; | ||
client.hydrateRoot = function(c, h, o) { | ||
i.usingClientEntryPoint = true; | ||
try { | ||
return m.hydrateRoot(c, h, o); | ||
} finally { | ||
i.usingClientEntryPoint = false; | ||
} | ||
}; | ||
} | ||
return 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.
This is the code from react-dom about handling things differently between development and prod envs. It requires react-dom
and not react-dom/client
, as explained in previous PRs.
Only the production path is handled, less code, smaller file, ESBuild does not need anymore to import react-dom
with the additional logic, but only react-dom/client
Added some comments to help the review |
const distDir = path.join(packageRoot, 'dist'); | ||
const packageData = await readPackageJSON(path.join(packageRoot, 'package.json')); | ||
const isStimulusBundle = '@symfony/stimulus-bundle' === packageData.name; | ||
const isReactOrVueOrSvelte = ['@symfony/ux-react', '@symfony/ux-vue', '@symfony/ux-svelte'].some(name => packageData.name.startsWith(name)); |
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.
const isReactOrVueOrSvelte = ['@symfony/ux-react', '@symfony/ux-vue', '@symfony/ux-svelte'].some(name => packageData.name.startsWith(name)); | |
const isReactOrVueOrSvelte |
hahaha i may have use a... another const name
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.
Yeah I wasn't really inspired 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.
The LiveComponent changes are a mess to review :)
…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
Following #2935. Same goals, but less frictions than with tsdown, tsup:
target
is correctly read from ourtsconfig.packages.json
//#region
commentstarget
stayses2021
without any issues withstatic
and StimulusTwo things:
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.d.ts
files removed, that's still fine, there is no point to generatedist/<file>.d.ts
files whendist/<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:
When watching LiveComponent assets, the CSS is easily watched too!
Enregistrement.de.l.ecran.2025-07-27.a.00.37.45.mov