Do not transpile JS packages twice #44824
Conversation
scinos
added
the
[Status] Needs Review
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~4082 bytes removed 📉 [gzipped]) Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~2951 bytes added 📈 [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 (~482 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. |
scinos
force-pushed
the
try/dont-transpile-packages
branch
from
9d76bd8
to
7ab4cea
Compare
|
This PR is blocked because Jest doesn't know how to resolve packages with a I raised a PR with the changes required to make the custom resolver work and it just got accepted! As soon as that change is released and we bump Jest to the new version this PR will become unblocked. |
scinos
force-pushed
the
try/dont-transpile-packages
branch
from
7ab4cea
to
9680f61
Compare
scinos
requested review from
jsnajdr and
a team
and removed request for
a team
|
If we point the Normally, the CJS and ESM builds differ only in the module format: one uses CJS exports, the other uses the After this patch, the code shipped with CJS and ESM will be different. If we are OK with publishing untranspiled code to NPM, we can remove the package transpilation stuff completely, even for CJS. It's then up to the package consumer to figure out how to consume the sources. |
IMO that's a good thing. We continue to publish a ES5 version, packed as CJS in I think that has more value than shipping ES5 in CJS and ESM. |
|
An alternative would be to introduce a third property ( |
Yeah, that's |
That sounds good only until one starts to look more deeply at who these CJS consumers are and how they consume the package in practice. If the consumer uses webpack, then webpack always looks at The change from transpiled to untranspiled requires caution on the consumer side. E.g., Jetpack uses The only place where I can see the CJS version really used is To summarize, I think there is a mismatch between what our packages expose and how the consumers actually consume it. That mismatch exists even without the changes in this PR -- the PR only highlights them. |
That's the default behaviour, but I'd say that's the consumer's responsibility to change if they are not ok with it. I do agree most people won't change it so for them,
For the public packages, we could bump the major version number and document the breaking change.
AFAIK, node always ignores the I think we have three options:
Sounds like (2) is the most sensible approach right now. We just need a name. |
|
We can also specify the requirements by consumers and then work from there. For browsers, I think the only practical consumer are bundlers, and to these, we can simply ship the untranspiled sources (even TypeScript) and shift the responsibility entirely to the consumer, i.e., the webpack config author. It's the consumer, not the package publisher, who knows what their target browsers and polyfills are. Transpilation by publishers only leads to suboptimal code (i.e., transpiled too much). Then there's the Node.js runtime that imports the package at runtime using the Node.js resolver and loader. Node.js doesn't understand JSX, so some transpilation by publisher will always need to happen. (Here the typical package is a React component that's also used for SSR in Node.) For Node.js without ESM and For latest Node.js with ESM unflagged, we need an ESM export with the same code content as CJS. Can all that be encoded in
Is there support in Webpack 5? We can write a webpack plugin for
This standardization debate has been going on for years, with no results. I'm afraid we'll need to go with something that's not standard, but is reasonably well designed and has a chance for wider adoption.
This article proposes |
|
When exposing the different exports from a package, my experience from #43679 has been that the biggest bottleneck is the TypeScript compiler. It has its own resolver which is not configurable. It's possible to write your own resolver, but that involves implementing the "compiler host" interface and somehow wiring it all together. It affects JS-only packages, too, as long as a TS source imports them. The TS compiler will look for the |
The article also proposes @jsnajdr In your opinion, is the name of the export the only thing that needs to be changed to get this merged to master? Or is there something else? |
I'd like to discuss and design the package-export strategy as a whole before starting to ship code. Right now, I'm working on understanding the conditional exports proposal more deeply. These are big changes and I think that trying to merge this or #43679 as expediently as possible is not the right focus. |
|
It seems that the current patch is able to satisfy the requirements in #44824 (comment): The When loaded natively by Node.js, the No custom field ( Open questions:
|
|
@scinos How do you think this PR is related to #43679 which has a similar goal? They both now use I didn't look at this problem in the last few months, so I forgot almost everything I knew about both PRs 🙂 |
Yeah, that's superseded by the Thanks for reviving this effort -- it's an important improvement that's been dormant for a few months unfortunately 🙁 |
|
Strange, I expected all bundles to be smaller as they are transpiled for evergreen now, but somehow some sections got bigger 🤨 . I'll investigate |
False alarm. Changes in chunk sizes are caused by webpack heuristics moving things around, but overall, this PR makes evergreen build ~30kb smaller (parsed size, from http://iscalypsofastyet.com/p/branch?branch=try/dont-transpile-packages). Manually inspecting a few compiled files I can see the diff comes, as expected, to a more effective transpilation (eg: there are no more |
|
Just FYI that I created a new package, |
|
@sirbrillig Thank you for letting me know :) I'll check it out and include it in this PR if needed |
scinos
force-pushed
the
try/dont-transpile-packages
branch
3 times, most recently
from
cc653e8
to
e00b567
Compare
There was a problem hiding this comment.
@scinos would you mind if I pushed a few little changes to this branch? Locally, I did a rebase to latest master and removed the packagesInMonorepo code, as the same thing has been merged this week together with #45774. I guess that I could just push it when I already did the work locally, in order to test the PR.
To confirm my understanding: this PR adds calypso:src and skips build for packages that are written in standard JS and transpiled using Babel. It keeps TypeScript packages unchanged, mainly because of the troubles with configuring a custom resolver for TypeScript compiler. In #43679 I tried to remove the build step for TypeScript packages, too, but that works only when they are compiled with Babel (with TS plugin). tsc doesn't find the imported modules.
| @@ -2,4 +2,5 @@ module.exports = { | |||
| preset: '@automattic/calypso-build', | |||
| rootDir: __dirname, | |||
| cacheDirectory: '<rootDir>/../../.cache/jest', | |||
| resolver: '<rootDir>/../../test/module-resolver.js', | |||
There was a problem hiding this comment.
This would be nice to move to some shared preset. The packages' Jest configs are getting repetitive: the cacheDirectory is always the same, and now the resolver field is, too.
By the way, even the rootDir field could be removed one day, when this Jest bug gets fixed.
There was a problem hiding this comment.
I couldn't figure out how to do it. When I added the cacheDirectory to the base jest config I got some problems relative to how jest resolves paths... I don't remember the details, I'll try again.
There was a problem hiding this comment.
Created a separate PR for it: #46953
test/module-resolver.js
Outdated
| packageFilter: ( pkg ) => { | ||
| //TODO: when Jest moves to resolver v2, this method will receive a second argument that points to the real package file (ie: no symlink) | ||
| //We can use it to determine if the package file is within the repo | ||
| if ( packages.includes( pkg.name ) ) { |
There was a problem hiding this comment.
Is this filtering by name even needed? What if we just checked if calypso:src field is present in package.json and returned a patched main if it is?
There was a problem hiding this comment.
Good point, probably not needed, I'll remove it.
scinos
force-pushed
the
try/dont-transpile-packages
branch
from
e00b567
to
8787b52
Compare
Of course I don't mind! :) Feel free to improve it.
I've made those changes as well.
Correct. |
Yeah, you already made the changes I had in mind 🙂 I have nothing further to add. |
matticbot
removed
the
[Status] Needs Review
Summary
Do not transpile some packages at installation time because webpack/jest will transpile them anyway. This allows faster installations (~38% faster) and smaller bundles.
Based on #43679
Background
Packages in
./packages/implement apreparescript that is run every time we doyarn install. For JavaScript packages, this script is using babel to transpile the source to CJS and ESM. Those packages expose the ESM result inpkg.module(used by Webpack) and the CJS result inpkg.main(used by Node).Later, when compiling
./client, webpack loads the ESM transpiled files, transpiles them again and add them to the bundles.Changes
Do not implement a
preparescript. Instead, use theprepackscript to transpile to CJS. This will run the transpilation whenyarn publishoryarn pack, but not on installation (see https://docs.npmjs.com/misc/scripts)Change
moduleto point to the source code (not the transpiled ESM). This allows distributing the untranspiled source code so consumers of this package (including Calypso) have control over the transpilation options. Specifically, this allows Calypso to transpile them for "evergreen" browsers. Before this change, packages in./packageswere always transpiled for "fallback" browsers.Drop the
preparescript. This will allow fasteryarn build-packages(which is run on everyyarn install). In my machine, in masteryarn build-packagestakes 39.9s to "prepare" 23 packages. After this change, it takes 24.6s (38% faster) to prepare 10 packages.Testing instructions
Update (22/Oct)
module, this PR introduces a newcalypso:srcfield that points to the untranspiled source code.pkg.moduleandpkg.mainare unchanged.