-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Consume monorepo packages without building them #43679
base: trunk
Are you sure you want to change the base?
Conversation
Caution: This PR affects files in the FSE Plugin on WordPress.com D45477-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing in the FSE Plugin for more info: PCYsg-ly5-p2 |
e7fabb6
to
3bf05d8
Compare
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Webpack Runtime (~193 bytes removed 📉 [gzipped])
Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded. App Entrypoints (~10235 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~24051 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 (~15528 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. |
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.
WordPress Desktop CI Failure (ci/wp-desktop): @jsnajdr please re-try this workflow ("Rerun Workflow from Failed") and/or review this PR for breaking changes. Please also ensure this branch is rebased off latest Calypso master.
Haven't seen this one before:
|
3bf05d8
to
ebee6f5
Compare
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.
Thank you so much for this Jarda!
I reviewed this and it seems to build correctly and works and is faster!
I investigated the caveats you mentioned a bit and I think I've come up with a solution for TypeScript.
TypeScript
Currently, TypeScript fails because types
field in package.json
(of a given package) points to a possibly non-existing file. Changing types
field to the source file (src/index.ts
) fixes the issue but is bad because if the package is ever published, src
theoretically won't be published with it.
An OK solution is to add an index.ts
file in the root of every TS pacakge. It should only be:
export * from './src/index';
💡 And npmignore
this file.
This works because TS mimics the Node resolving algorithm, an index.ts
file will tip it off to find the types file (in src) before failing over to types
field in package.json
. And when the package is published, index.ts
won't exist, and types
field will take precedence and the types in dist
will be used.
Maybe we can create a nice comment and add it inside this index.ts
file to explain its existence.
clean:packages
Currently, there is a still a package.json script called "clean:packages". It runs when node_modules is outdated and it takes some time. I think this can go 🎉
calypso-color-schemes
The only package that still needs the build step is calypso-color-schemes
I think a fair solution is to rename its output to something other than dist
and remove the output files from gitignore
. The output is CSS (or the equivalent), I don't think it will be OS or Node version dependant. No need to rebuild them on everyone's machine.
If you'd like me to execute these changes I'd be happy to.
@jsnajdr: the bundle size savings in this PR are interesting. My guess is that they come from packages now being compiled with |
Yes, I didn't investigate it in detail, but I think the savings come from two places:
I'm not sure if the second thing (no |
That would require the resolver to first look for But this If the After implementing this though, I have a little problem with the
Apparently, if the @alshakero @razvanpapadopol I assume that the "types patch" is there to monkey-patch a bug in the |
TIL. I added
That's right.
Sounds like a good solution |
The If you load the TypeScript compiler programmatically from the But configuring the What's kind of a good news is that the Yarn PnP project needs to do the same thing as we do here: pass a custom resolver to various tools. We're not alone. Yarn PnP works with webpack, Maybe we could stop using
|
ebee6f5
to
d77465d
Compare
…repo package at runtime by Node
…ackages on every Calypso build
d77465d
to
e17b6ee
Compare
A few notes:
|
Raised #44824 to cover some of the intent of this PR (do not build JS packages, leave TS packages for now) |
If I understand it correctly, that's now solved with your Jest PR and your Jest resolver in #44824 uses the same technique as the one in this PR?
Yes, I agree that it's too big for merging at once. My intent was to figure out if there's a solution that works for the whole codebase, without any errors, and then implement it incrementally. It would be a waste to implement some technique only to discover there is a roadblock that prevents us from using it universally.
Beware that there are two kinds of "transpilation" that are orthogonal to each other:
The CJS and ESM bits will be completely removed by the bundler (webpack), but the remaining code will stay and will be shipped to the destination (a browser or a Node runtime). |
Few weeks ago, while on leave, I found this article that suggests using the modern Looks like something definitely worth looking at. |
This PR has been marked as stale due to lack of activity within the last 30 days. |
This PR makes it possible to consume monorepo packages (
packages/*
) without building them. To build Calypso, i.e., its webpack bundle, thedist/esm
anddist/cjs
folders don't need to be created and populated with transpiled files. That's a task that's needed only when preparing a package to be published in the NPM registry.Instead, the Calypso webpack build can now directly consume the sources in
src/
, and transpile any Babel-enhanced JavaScript or TypeScript with its webpack loaders, just like any other non-package file inclient/
.I achieved this by adding
calypso:main
field to all packages, and pointing in to the./src/index
source file. Then I configured the webpack resolver to look atcalypso:main
first, using theresolve.mainFields
option.To make tests work, the Jest resolver needs to be aware of
calypso:main
, too. That's harder, and I had to write a custom resolver (packages/calypso-build/jest/calypso-resolver
) by copying and modifying the default resolver from Jest sources.Building Calypso, both in development (with the dev middleware that watches the sources for changes) and production, now doesn't require the
build-packages
orbuild-packages:watch
steps. Simply running webpack is enough.The only package that still needs the build step is
calypso-color-schemes
. It does SCSS-to-CSS-and-JS compilation, and also performs some magic with copyingcolor-schemes
assets (I remember that @ockham had to add that at some point to solve some problem that didn't have any other solution)Risks:
There are several independent implementations of the module resolution algorithm in various tools:
And not all of them can be easily configured to look at the
calypso:main
fields. We might end up with some part of our build pipeline being unable to load the monorepo packages correctly.How to test:
Run
yarn start
and see if it builds Calypso correctly and if the build is faster now.