-
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
Make client/ directory (Calypso app) a Lerna package #38190
Conversation
5e67e16
to
3f442d9
Compare
"yargs": "13.3.0" | ||
"@automattic/full-site-editing": "file:apps/full-site-editing", | ||
"@automattic/wpcom-block-editor": "file:apps/wpcom-block-editor", | ||
"wp-calypso": "file: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.
if we go this route we should probably include everything in apps/*
?
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.
Yes, and also all the packages in packages/
that are not used by Calypso or something else in the repo transitively.
In other words, all packages in the monorepo should be direct or indirect dependencies of the root package.json
. That ensures that npm install
installs the whole package tree for the whole monorepo.
Gutenberg does it: https://github.com/WordPress/gutenberg/blob/master/package.json#L20-L66
Alternatively, lerna bootstrap
does the npm installation process, but bails out to a simple npm install
if it sees a local file:
ref in dependencies
in the root package.json
. We could also go that way, but leaving the npm tree solely to the npm client is preferrable.
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.
Alternatively, lerna bootstrap does the npm installation process, but bails out to a simple npm install if it sees a local file: ref in dependencies in the root package.json. We could also go that way, but leaving the npm tree solely to the npm client is preferrable.
Yeah, if we want to use lerna bootstrap, we'd also want to jump back to semver ranges instead of file refs. Once you add one file ref, bootstrap
stops doing anything useful.
If we really want a mix, something like yarn workspaces or pnpm workspaces offer a lot more flexibility.
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 way I've been thinking about it: lerna provides the "I want to run a command against the package graph" functionality and something else (npm, yarn, pnpm) provides the install and graph linkage. lerna bootstrap
is to be avoided as it's just a hacky way of doing what one of the package managers would rather do itself.
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.
Totally agreed that lerna bootstrap
is to be avoided. Even the Lerna maintainer told us that it's a buggy and hard-to-maintain hack that tries to do the package manager's job (poorly).
1404179
to
88ec205
Compare
cc @tyxla this PR might be relevant movement to follow regarding Jetpack Cloud living in |
88ec205
to
4368ec1
Compare
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~11720 bytes removed 📉 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~48194 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 (~5832 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. |
ae35311
to
5d707df
Compare
This seems to work pretty well at this moment and I think we are ready for review and testing 😊 |
5d707df
to
7dfdd71
Compare
Would we consider (possibly as a follow-up) |
@jsnajdr are there particular changes which could affect FSE here? (i.e. any idea what I should look out for when testing?) I see that the package-lock files for the individual apps are removed, which is nice :) |
The request string and the externals declaration must exactly match. That makes sure that `search-index.js` is not bundled into server bundle, but continues to be read with `require( 'server/bundler/search-index')`. Also removes legacy externals like `hot-reloader` or `bundler/assets` (the assets file is read with `fs.readFile`, not with Node require)
Don't specify a `packageDir` and make it look at the closest `package.json` by default. Also, add `lodash` as a top-level dev dependency, as some build-only files (e.g., `.eslintrc`) use it.
Allow `devDependencies` only in the root `package.json` and not anywhere else.
5db1083
to
9e9de37
Compare
👋 @jsnajdr I noticed that this PR moved the root Webpack configs to |
That's a good question! When I tried to move The A possible solution is to never allow the I don't know if the PostCSS file can be safely moved. I forgot about it when working on the patch, as the CSS build was never broken because of it 🙂 |
…I due to an update to the master eslintrc in #38190 Let's just override it for this Calypso dir
Turns the Calypso client (and server) into proper Lerna monorepo package. The top-level
package.json
now contains only dev dependencies for the whole monorepo, and there is a newclient/package.json
that now has the runtime dependencies for Calypso client and server.The main benefit is that we can now have a single unified
node_modules
tree, installed with onenpm ci
command, and with one globalpackage-lock.json
. Separate lockfiles and NPM installs inapps/
were removed. The only part of the monorepo that continues to have its own NPM tree istests/e2e
.The
server/
directory was moved toclient/server/
to make the Calypso app self-contained inside one directory. Many import paths in theserver/
sources needed to be updated and clarified.The root
package.json
declares the app-like subpackages asdependencies
to make them part of thenpm i
tree.Some
devDependencies
were incorrectly declared asdependencies
, so I moved them to the right place.Some areas that are not 100%
Dockerfile
, I had to disable the separate layer fornpm ci
. If I understand the logic correctly, it first copies a minimal version of the source tree that's needed to run thenpm install
, then installs thenode_modules
, and only then copies and builds the rest? @sirreal it would be nice if we could make it work againeslint-plugin-import
errors reported by ESLint now than needed. Can we improve on that?wp-desktop
is failing. 🙂How to test: