Use babel to transpile TS projects #47502
Conversation
matticbot
added
the
[Status] Needs Review
| "prepublish": "yarn run clean", | ||
| "prepare": "tsc --build ./tsconfig.json && tsc --build ./tsconfig-cjs.json", | ||
| "watch": "tsc --build ./tsconfig.json --watch" | ||
| "clean": "tsc --build ./tsconfig.json ./tsconfig-cjs.json --clean && npx rimraf dist", |
There was a problem hiding this comment.
tsc --build ... --clean deletes the files but lefts lots of empty dirs, that's why I added npx rimraf dist
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~8266 bytes removed 📉 [gzipped]) Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~26680 bytes removed 📉 [gzipped]) Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to. Async-loaded Components (~22061 bytes removed 📉 [gzipped]) React components that are loaded lazily, when a certain part of UI is displayed for the first time. Legend What is parsed and gzip size?Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory. Generated by performance advisor bot at iscalypsofastyet.com. |
|
About bundle size: I diffed a few files and the biggest difference come from bundling and using Before this PR we bundled |
| @@ -25,7 +25,7 @@ | |||
| }, | |||
| "scripts": { | |||
| "clean": "npx rimraf js css src/__color-studio", | |||
| "prepare": "node bin/prepare-sass-assets.js && node bin/build-css.js" | |||
| "postinstall": "node bin/prepare-sass-assets.js && node bin/build-css.js" | |||
There was a problem hiding this comment.
Not sure about this change tbh.
Calypso needs those commands before it can use the package, so postinstall seems natural. However any external consumer of @automattic/calypso-color-schemes will also run postinstall, which will fail if they have not installed the dev dependencies of the package.
We could keep prepare and bring back running prepare on Calypso's postinstall, but that means we have to use lerna just to run prepare in a couple of packages.
There was a problem hiding this comment.
Does yarn workspaces run prepare work to avoid lerna?
It looks like yarn uses different lifecycle scripts, should we be updating these? prepare is a holdover from npm days as far as I can tell…
There was a problem hiding this comment.
yarn workspaces run prepare fails if any package in the workspace does not implement a prepare script.
Yarn runs prepare (at least yarn v1), but only for the root package, it doesn't run it for the subpackages (yarnpkg/yarn#3911).
So either we have our own explicit list ("prepare": "yarn workspace @automattic/calypso-color-schemes run prepare && yarn workspace @automattic/languages run prepare"), use a different tool like wsrun or stick with lerna.
| { "path": "../packages/data-stores" }, | ||
| { "path": "../packages/language-picker" }, | ||
| { "path": "../packages/react-i18n" } | ||
| ], |
There was a problem hiding this comment.
There's also a packages/languages package that is written in TypeScript and isn't being migrated in this PR: it's still built during the prepare step.
There was a problem hiding this comment.
packages/languages is migrated in the sense that has calypso:src, but still requires a build step because we do use it to build Calypso: yarn build-languages calls bin/build-languages.js which requires '@automattic/languages'
There was a problem hiding this comment.
Oh, I see, the package is used by a Node.js build script and needs to be built 🙁
There was a problem hiding this comment.
The @automattic/languages is in a very similar situation as a Gutenberg webpack plugin we're discussing with @sirreal in WordPress/gutenberg#26382 (comment)
I think we'd be better off if the package was written in plain JavaScript, where it would be a trivial two-liner:
import data from './languages-meta.json';
export default Object.values( data );and if the types were in a separate index.d.ts file.
Then the package wouldn't need any build step at all, could be directly consumed by Node.js build scripts, and consumers would still get TypeScript types for it without any observable change.
This approach is an exception rather that a rule, but I think it would be justified for certain packages like these I mention.
This is a nice benefit. Packages are also configured inconsistently so we were likely getting both the bundled tslib and some number of inline or per-module tslib definitions. Ideally all the packages build using tslib. |
| @@ -7,7 +7,7 @@ | |||
| "jsx": "react", | |||
| "declaration": true, | |||
| "declarationDir": "dist/types", | |||
| "outDir": "dist/types", | |||
| "outDir": "dist/esm", | |||
There was a problem hiding this comment.
I pushed a fix, esm build was being written to the wrong place 🙂
|
Super fast 👍 |
There was a problem hiding this comment.
No problems with checkout that I can detect. And as for the building speed... 😮 I've never seen yarn complete so fast.
Update: I also tested running the branch and modifying a file in a checkout-related package, letting it rebuild, and then making sure the change was included. All worked as expected.
matticbot
removed
the
[Status] Needs Review
Summary of changes
babelto transpile TypeScript packages at bundle time.tscto check types and generate type definitions.Inspired by the discussion in #47002 by @jsnajdr and @sirreal
Background
Our Babel config can transpile TypeScript files. However we are not using that capability. Instead, we have a script
build-packagesthat runstscon each package, creating an ESM compilation. Then that compilation is read by webpack and bundled with the rest of the app. This is not optimal because TypeScript can't do a smarter transpilation based on the target (evergreen vs fallback), and also it wastes time as packages are transpiled twice.Changes
Exposes the untranspiled TypeScript code in TS packages using the property
calypso:srcthat we introduced in Do not transpile JS packages twice #44824. This allows Webpack and babel to read the original TypeScript code and transpile it.Create a
buildstep on each TypeScript package that will generate CJS, ESM and types. This is used onprepack, when the package is being built for publishing.Create a "main"
tsconfig.jsonthat references all TS packages, used as an entrypoint fortsc.Notes
We don't really need to run
tscat all for Calypso to compile and work. It is all handled by babel. The only reason to runtscat installation time is to generate type definitions that can be used by the IDE.In theory we could use just
tsc --emitDeclarationOnlyto generate the definition files. However,tsc --buildalso generates the types and it is much faster with a warm cache.I don't think we need watch mode for packages (i.e.
build-packages:watch). In my experiments, if you run webpack in watch mode and change a.tsfile in a package, webpack will rebuild as expected. Also, if you change a type in a TS package, VSCode will pick the change, no need to rebuild the types. However, cleaning up watch mode is left for a future PR to avoid scope creep.Testing instructions
Verify tests are green, ICFY doesn't show a crazy diff in bundle size and smoke test calypso.live (in Chrome and IE11), specially areas that have been moved to TS packages (eg: language picker, checkout, shopping cart)
Checkout this branch and run
yarntwice. The second time (deps already installed, nothing to build) should finish in a couple of seconds.