Build tooling changes #2922
Build tooling changes #2922
Conversation
vladmoroz
requested review from
benoitgrelard,
andy-hook and
lucasmotta
as code owners
| "// build": "For context on tsconfig replacements in build scripts, see https://github.com/radix-ui/primitives/pull/361#discussion_r555004944", | ||
| "build": "yarn build:config && yarn build:packages && yarn build:cleanup", | ||
| "build:config": "mv tsconfig.json tsconfig.tmp.json && mv tsconfig.production.json tsconfig.json", |
There was a problem hiding this comment.
This dance with configs isn't needed anymore
| module.exports = { | ||
| presets: [ | ||
| [ | ||
| '@parcel/babel-preset-env', | ||
| { | ||
| bugfixes: true, | ||
| targets: { | ||
| browsers: 'Chrome >= 74, Safari >= 13.1, iOS >= 13.3, Firefox >= 78, Edge >= 79', | ||
| node: 12, | ||
| }, | ||
| }, | ||
| ], | ||
| '@babel/preset-react', | ||
| ], |
There was a problem hiding this comment.
Downtranspilation is retained in esbuild via a newer ES2022 target. Realistically, with the JS features we use, we'd support browsers up to 4 years old.
| const pureDisplayNames = () => ({ | ||
| visitor: { | ||
| AssignmentExpression(path) { | ||
| if ( | ||
| path.node.left.type === 'MemberExpression' && | ||
| path.node.left.property.name === 'displayName' && | ||
| path.node.right.name | ||
| ) { | ||
| const COMPONENT = path.node.left.object.name; | ||
| const DISPLAY_NAME = path.node.right.name; | ||
| const ast = buildAssign({ COMPONENT, DISPLAY_NAME }); | ||
| path.replaceWith(ast); | ||
| path.addComment('leading', '#__PURE__'); | ||
| } | ||
| }, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Have dug up some history on this one:
Since each package is bundled into a single file, pure annotations were added in key places to support tree-shaking. This doesn't seem bulletproof, as I've seen them missing in some places, and not every build tool supports the annotations.
I'm thinking that instead, we should make tree-shaking possible naturally via:
- Have a file per part
- Don't bundle
That will allow build tools to include or omit the relevant imports naturally, and allow us to simplify our toolchain as well. We've already worked out how to do that in Radix Themes, including dual CJS/ESM in that setup (which is currently the only thing that bundling really simplifies), so I already have a good idea on the next steps.
| @@ -1,8 +1,6 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Had to please the pre-push hooks and remove the unused import (how has this stuff worked before?)
Other changes in this file are due to my formatting on save
| "build": "yarn build:config && yarn build:packages && yarn build:cleanup", | ||
| "build:config": "mv tsconfig.json tsconfig.tmp.json && mv tsconfig.production.json tsconfig.json", | ||
| "build:packages": "parcel build 'packages/*/*/' --no-cache && yarn build:fix-type-defs", | ||
| "build:fix-type-defs": "node ./scripts/fix-type-defs-imports", |
There was a problem hiding this comment.
No need to fix the bugged imports anymore
| // tsup is used to emit d.ts files only (esbuild can't do that). | ||
| // | ||
| // Notes: | ||
| // 1. Emitting d.ts files is super slow for whatever reason. | ||
| // 2. It could have fully replaced esbuild (as it uses that internally), | ||
| // but at the moment its esbuild version is somewhat outdated. | ||
| // It’s also harder to configure and esbuild docs are more thorough. | ||
| await tsup.build({ | ||
| entry: [file], | ||
| format: ['cjs', 'esm'], | ||
| dts: { only: true }, | ||
| outDir: dist, | ||
| silent: true, | ||
| external: [/@radix-ui\/.+/], | ||
| }); | ||
| console.log(`Built ${path}/dist/index.d.ts`); | ||
| } |
There was a problem hiding this comment.
tsup gets the job done, but this is temporary—we'll probably just use tsc to compile the type definitions when stuff is updated to not need bundling in the first place:
#2922 (comment)
| swc: () => ({ | ||
| jsc: { | ||
| transform: { | ||
| react: { | ||
| // Do not require importing React into scope to use JSX | ||
| runtime: 'automatic', | ||
| }, | ||
| }, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
Somehow:
- pre-push hooks didn't like unused React imports in some stories (how did folks push to their branches before?)
- Stories without React in scope won't compile
|
@hadihallak yep, I confirmed that "splitting" and React in the external don't do anything right now, this was a remnant from a different setup I was exploring when these were useful |
Changing build tooling to esbuild and (temporarily) tsup in order to support
'use client'directives up next. (Currently, Parcel strips them out, see parcel-bundler/parcel#9050). Also sets us free from a bunch of other workarounds related to the current setup.