Update WordPress dependencies #35729

jsnajdr · 2019-08-23T11:51:32Z

Big cleanup of @wordpress dependencies in Calypso monorepo:

In full-site-editing and wpcom-block-editor, remove @wordpress dependencies and other dependencies that are externalized by the dependency-extraction-webpack-plugin (lodash, jquery, react, ...) from package.json. These don't need to be installed in node_modules, because they are never accessed from there. Keep installing only the deps that are actually bundled (classnames, debug). This change also prevents Renovate Bot from submitting nonsensical "pin dependencies" PR like #35216.

This PR also removes full-site-editing/package-lock.json: it's not needed -- all locking is done in the root of the monorepo.

In Calypso, upgrade all @wordpress dependencies (we actually bundle them there) to the latest working version (we can't upgrade to the very latest ones because of WordPress/gutenberg#17165). And enumerate them all, even the transitive ones, to ensure that we don't get duplicate copies of packages bundled with different versions.

Last but not least, pin the @wordpress/dependency-extraction-webpack-plugin. That should make #35216 completely obsolete for now.

How to test:

verify that full-site-editing and wpcom-block-editor build correctly and work correctly
verify that Calypso is OK and that the Duplicate Package Checker webpack plugin no longer warns about @wordpress packages.

Fixes #35497.

matticbot · 2019-08-23T11:51:36Z

Test live: https://calypso.live/?branch=update/wordpress-deps

matticbot · 2019-08-23T11:59:45Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~3365 bytes added 📈 [gzipped])

name                parsed_size           gzip_size
gutenberg-editor        -2516 B  (-0.4%)     -441 B  (-0.2%)
woocommerce               +23 B  (+0.0%)     +173 B  (+0.0%)
themes                    +23 B  (+0.0%)     +173 B  (+0.2%)
stats                     +23 B  (+0.0%)     +173 B  (+0.1%)
settings-writing          +23 B  (+0.0%)     +173 B  (+0.1%)
security                  +23 B  (+0.0%)     +173 B  (+0.2%)
purchases                 +23 B  (+0.0%)     +173 B  (+0.1%)
posts-pages               +23 B  (+0.0%)     +173 B  (+0.2%)
posts-custom              +23 B  (+0.0%)     +173 B  (+0.2%)
post-editor               +23 B  (+0.0%)     +173 B  (+0.0%)
plans                     +23 B  (+0.0%)     +173 B  (+0.1%)
people                    +23 B  (+0.0%)     +173 B  (+0.2%)
login                     +23 B  (+0.0%)     +173 B  (+0.4%)
jetpack-connect           +23 B  (+0.0%)     +173 B  (+0.1%)
help                      +23 B  (+0.0%)     +173 B  (+0.1%)
google-my-business        +23 B  (+0.0%)     +173 B  (+0.2%)
export                    +23 B  (+0.0%)     +173 B  (+0.2%)
domains                   +23 B  (+0.0%)     +173 B  (+0.1%)
concierge                 +23 B  (+0.0%)     +173 B  (+0.2%)
comments                  +23 B  (+0.0%)     +173 B  (+0.1%)
checkout                  +23 B  (+0.0%)     +173 B  (+0.1%)
activity                  +23 B  (+0.0%)     +173 B  (+0.2%)
account                   +23 B  (+0.0%)     +173 B  (+0.2%)

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 (~1730 bytes added 📈 [gzipped])

name                                        parsed_size           gzip_size
async-load-signup-steps-plans-atomic-store        +23 B  (+0.0%)     +173 B  (+0.6%)
async-load-signup-steps-plans                     +23 B  (+0.0%)     +173 B  (+0.5%)
async-load-signup-steps-domains                   +23 B  (+0.0%)     +173 B  (+0.3%)
async-load-signup-steps-clone-point               +23 B  (+0.0%)     +173 B  (+0.4%)
async-load-reader-search-stream                   +23 B  (+0.0%)     +173 B  (+0.6%)
async-load-reader-following-manage                +23 B  (+0.0%)     +173 B  (+0.5%)
async-load-design-playground                      +23 B  (+0.0%)     +173 B  (+0.0%)
async-load-design-blocks                          +23 B  (+0.0%)     +173 B  (+0.0%)
async-load-design                                 +23 B  (+0.0%)     +173 B  (+0.0%)
async-load-blocks-inline-help-popover             +23 B  (+0.0%)     +173 B  (+0.2%)

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.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

simison · 2019-08-23T12:21:32Z

Yasss! Let's do this.

Only that now Eslint warns about import/no-extraneous-dependencies rule. :-(

jsnajdr · 2019-08-23T12:29:33Z

Only that now Eslint warns about import/no-extraneous-dependencies rule.

In the FSE sources? Or somewhere else? Hopefully we can teach the rule about the externalized deps 🙂

simison · 2019-08-23T12:46:21Z

In the FSE sources?

Yeah:

npx eslint apps/wpcom-block-editor/ apps/full-site-editing/

simison · 2019-08-23T12:47:50Z

Does this affect folks working with TypeScript? cc @Automattic/type-review

jsnajdr · 2019-08-23T13:11:09Z

Does this affect folks working with TypeScript?

That's an interesting question. We don't bundle the code, but need the types. The types are not in the @wordpress/* packages themselves, but rather in the @types scope (DefinitelyTyped repo). Does any tool break if the code is not present in package.json and node_modules?

simison · 2019-08-23T14:47:39Z

There might've been talks about bringing Types to @wordpress/* packages if I remember correctly. Someone else might fill in with links.

Does any tool break if the code is not present in package.json and node_modules?

...and if so, would it help to add them to package.json's optionalDependencies with * version range?

noahtallen · 2019-08-23T20:54:16Z

I pulled this branch, and it looks like full site editing is building as expected :)

The lint issue would definitely be annoying though. I tried this solution: import-js/eslint-plugin-import#458 (comment) but Eslint didn't accept it as a valid rule.

mmtr · 2019-08-24T02:28:09Z

Tested that wpcom-block-editor builds are still generated as expected 👍

Only that now Eslint warns about import/no-extraneous-dependencies rule.

In the FSE sources? Or somewhere else? Hopefully we can teach the rule about the externalized deps 🙂

Seems there are no options for ignoring specific dependencies, so we'll need to disable the rule individually on each import by adding the // eslint-disable-line import/no-extraneous-dependencies comment.

noahtallen · 2019-08-24T18:05:07Z

so we'll need to disable the rule individually on each import

I feel like there's got to be a better way 😅 That would be a lot of lines to disable!!

simison · 2019-08-26T07:01:41Z

packages/calypso-build/package.json

@@ -45,7 +45,7 @@
 		"@babel/preset-typescript": "7.3.3",
 		"@wordpress/babel-plugin-import-jsx-pragma": "2.3.0",
 		"@wordpress/browserslist-config": "2.3.0",
-		"@wordpress/dependency-extraction-webpack-plugin": "^1.0.1",
+		"@wordpress/dependency-extraction-webpack-plugin": "1.1.0",


If you get blocked by eslint issue with removing deps, this change alone could be in separate PR.

Do we need to pin this exact version?

Isn't it the latest? (unless I misunderstand your question?)

The question was more about removing the ^ - exact version vs. version range.

Not a big deal, it's a dependency that I can't imagine ever being duplicated into a bundle that's shipped to a browser. But the version range allows for deduplication. From memory, I don't think there have been significant changes since 1.0.1

sirreal · 2019-08-26T08:12:54Z

This PR also removes full-site-editing/package-lock.json: it's not needed -- all locking is done in the root of the monorepo.

👍 I think this is accurate, I don't believe that package-lock file had any impact with the we install packages at this time, but it does raise an interesting point. We don't have package-lock in packages/* or apps/*, and it's working fine. However packages are part of the calypso dependency graph, whereas apps like full-site-editing live outside it. That means that npm install will reach dependencies of packages and install them, but not dependencies of isolated apps like full-site-editing. This is fine because the dependencies happen to overlap right now and can be resolved from the repository root, but that may not continue to be the case. We're relying on this overlap for apps to work.

This is something that lerna bootstrap addresses, it installs the dependencies of packages in the repo. We've been avoiding it so far because it adds complexity and I believe it was much slower than npm install, but we may need to reconsider how apps manage their dependencies.

sirreal · 2019-08-26T08:13:04Z

Only that now Eslint warns about import/no-extraneous-dependencies rule.

In the FSE sources? Or somewhere else? Hopefully we can teach the rule about the externalized deps 🙂

We might be able to improve this behavior with a webpack resolver for the rule: https://github.com/benmosher/eslint-plugin-import#resolvers

sirreal · 2019-08-26T09:41:34Z

Does this affect folks working with TypeScript?

That's an interesting question. We don't bundle the code, but need the types. The types are not in the @wordpress/* packages themselves, but rather in the @types scope (DefinitelyTyped repo). Does any tool break if the code is not present in package.json and node_modules?

I don't think this breaks any tooling. TypeScript complains that it can't find the types, but it's doing a lot of complaining right now as part of our gradual typing process 🙂 TypeScript might have been able to parse the JSDoc from @wordpress dependencies, but I don't think that's a concern.

I don't think we're using @wordpress dependencies in any typed files at this time anyways. I'd suggest that folks can add just the types as devDependencies when and if they desire:

npm i -D npm install --save @types/wordpress__{blocks,components,editor,i18n,element}

sirreal · 2019-08-26T11:03:56Z

There seem to be some regular e2e failures. I'm not sure whether this branch regresses or they were preexisting.

jsnajdr · 2019-08-26T11:27:52Z

We might be able to improve this behavior with a webpack resolver for the rule:

@sirreal I tried to declare @wordpress/data as a core module that's part of the platform (which is true here, because it's part of the enviroment where a WordPress plugin runs):

settings: {
  'import/core-modules': [
    '@wordpress/data'
  ]
}

But then the import/no-nodejs-modules started complaining about importing core modules 🤷‍♂

I could probably fix that with the allow option, but then I'm in a position where I'm duplicating the list of external modules at three places rather than doing anything useful.

What kind of bugs did the import/* rules help us fix? Most of the time I encounter them, they don't report any real issue and just produce noise. Like in this PR.

jsnajdr · 2019-08-26T11:32:49Z

There might've been talks about bringing Types to @wordpress/* packages if I remember correctly. Someone else might fill in with links.

Then we'll have to add them back as dev dependencies at least.

...and if so, would it help to add them to package.json's optionalDependencies with * version range?

That would make the packages appear in the node_modules folder, but it's not what optionalDependencies are for. "optional" means that the dependency can fail to install (e.g., the package is not available for given platform) and the project can handle that and continue working nevertheless.

…remaining ones

…ct) and pin remaining ones

Upgrade WordPress deps to latest working version and explicitly pin versions to avoid duplicate sub-packages with different versions.

jsnajdr · 2019-08-26T12:19:08Z

I think this is accurate, I don't believe that package-lock file had any impact with the we install packages at this time, but it does raise an interesting point. We don't have package-lock in packages/* or apps/*, and it's working fine. However packages are part of the calypso dependency graph, whereas apps like full-site-editing live outside it.

@sirreal you're totally right! We don't need lockfiles in packages/*, because their dependencies are installed and locked transitively when doing npm install in the root directory and following the file:./packages/... links.

But projects in apps/* are root projects that need to do their own npm install. That would become completely obvious at the moment when Calypso app gets moved to apps/calypso. Then there's no root package.json or node_modules any more and all apps/* must rely on themselves.

Solvable with Lerna Bootstrap or Yarn Workspaces. Neither is ideal for us ☹️

I added the package locks for full-site-editing and wpcom-block-editor back in 4fc70c7.

apps/notifications remains to be an offender: it doesn't declare any dependencies in its package.json at all, and doesn't have a lockfile. Also, it's both a standalone app (shipped to WPCOM) and a module (imported by Calypso), which doesn't make things easier. Standalone app should have a lockfile, a module shouldn't.

simison · 2019-08-26T12:44:39Z

apps/notifications remains to be an offender: it doesn't declare any dependencies in its package.json at all, and doesn't have a lockfile. Also, it's both a standalone app (shipped to WPCOM) and a module (imported by Calypso), which doesn't make things easier. Standalone app should have a lockfile, a module shouldn't.

You'll need an app notifications in /apps which imports the module notifications from /packages? 😁

sirreal · 2019-08-27T11:43:59Z

Full e2e pass:

(desktop) https://circleci.com/gh/Automattic/wp-e2e-tests-for-branches/22035

sirreal · 2019-08-27T11:51:53Z

What kind of bugs did the import/* rules help us fix? Most of the time I encounter them, they don't report any real issue and just produce noise. Like in this PR.

They ensure that packages do declare their dependencies. In the monorepo, if a package's dependency can be resolved from the root, the package works fine. However, if the published version does not declare its dependencies properly, the package may be unusable.

I don't know whether we've actually had bugs introduced that needed to be fixed, but I know that is has helped me declare dependencies correctly for packages.

gwwar · 2019-08-27T17:32:48Z

🤔 are we making this too complicated for ourselves? Should we just use the wp script notation instead, off of the window object? This is what webpack is rewriting this to in the final script. I'd probably prefer that over needing to disable the eslint rule individually on each import

sirreal

Full e2e now seem to be passing, this looks good to me, thanks!

Some interesting questions have come up around the lint rule and the way isolated app dependencies are installed, but they're beyond the scope of this PR.

sirreal · 2019-08-27T21:59:05Z

packages/calypso-build/package.json

@@ -45,7 +45,7 @@
 		"@babel/preset-typescript": "7.3.3",
 		"@wordpress/babel-plugin-import-jsx-pragma": "2.3.0",
 		"@wordpress/browserslist-config": "2.3.0",
-		"@wordpress/dependency-extraction-webpack-plugin": "^1.0.1",
+		"@wordpress/dependency-extraction-webpack-plugin": "1.1.0",


Do we need to pin this exact version?

jsnajdr · 2019-08-29T07:52:55Z

Some interesting questions have come up around the lint rule and the way isolated app dependencies are installed, but they're beyond the scope of this PR.

@sirreal what do you think about disabling the import rules for the FSE and Block Editor apps for now? The ESLint rules are designed either for Node apps, or for mainstream React apps that bundle everything with webpack and are self-contained.

We do WordPress JS plugins, which run in a special environment where many imports are externalized. We can figure out the externals configuration (and possibly submit some PRs to the ESLint plugin) in another PR.

are we making this too complicated for ourselves? Should we just use the wp script notation instead, off of the window object?

@gwwar Using import '@wordpress/foo' and externalizing that in webpack is perfectly fine and there's nothing wrong about it. It's the ESLint rule that's not flexible enough. I'd prefer to bend the ESLint rule rather than being forced to stop things that are fine.

jsnajdr requested review from simison, ockham, sirreal, gwwar and a team August 23, 2019 11:51

jsnajdr requested a review from a team as a code owner August 23, 2019 11:51

jsnajdr self-assigned this Aug 23, 2019

jsnajdr added [Goal] Full Site Editing [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Build labels Aug 23, 2019

simison requested a review from a team August 23, 2019 11:56

simison added the Packages label Aug 23, 2019

mmtr mentioned this pull request Aug 24, 2019

(5P) SPT add JS Functional tests to the plugin #35379

Merged

simison reviewed Aug 26, 2019

View reviewed changes

sirreal added [Status] Needs e2e Testing and removed [Status] Needs e2e Testing labels Aug 26, 2019

jsnajdr added 7 commits August 26, 2019 13:55

Full Site Editing: remove externalized deps (WP, lodash) and pin the …

5d5bed4

…remaining ones

WPCOM Block Editor: remove externalized deps (WP, lodash, jquery, rea…

336b0cb

…ct) and pin remaining ones

Pin the wordpress/dependency-extraction-webpack-plugin version

0773232

Calypso: Upgrade and deduplicate WordPress deps

c846c99

Upgrade WordPress deps to latest working version and explicitly pin versions to avoid duplicate sub-packages with different versions.

Update lockfile

afc75fa

Full Site Editing: add calypso-build as devDependency

0cd3532

Update lockfiles in the apps/ directory

4fc70c7

jsnajdr force-pushed the update/wordpress-deps branch from 483206e to 4fc70c7 Compare August 26, 2019 12:09

sirreal approved these changes Aug 27, 2019

View reviewed changes

jsnajdr merged commit 9cab1a0 into master Aug 29, 2019

jsnajdr deleted the update/wordpress-deps branch August 29, 2019 07:53

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 29, 2019

Copons mentioned this pull request Sep 5, 2019

Full Site Editing: Add alignment options to the FSE blocks #35999

Merged

mmtr mentioned this pull request Jul 22, 2020

Adds missing dependencies to wpcom-block-editor #44341

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update WordPress dependencies #35729

Update WordPress dependencies #35729

jsnajdr commented Aug 23, 2019 •

edited

Loading

matticbot commented Aug 23, 2019

matticbot commented Aug 23, 2019 •

edited

Loading

simison commented Aug 23, 2019

jsnajdr commented Aug 23, 2019

simison commented Aug 23, 2019

simison commented Aug 23, 2019

jsnajdr commented Aug 23, 2019

simison commented Aug 23, 2019 •

edited

Loading

noahtallen commented Aug 23, 2019

mmtr commented Aug 24, 2019

noahtallen commented Aug 24, 2019

simison Aug 26, 2019

sirreal Aug 27, 2019

simison Aug 28, 2019

sirreal Aug 28, 2019

sirreal commented Aug 26, 2019 •

edited

Loading

sirreal commented Aug 26, 2019

sirreal commented Aug 26, 2019

sirreal commented Aug 26, 2019

jsnajdr commented Aug 26, 2019

jsnajdr commented Aug 26, 2019

jsnajdr commented Aug 26, 2019

simison commented Aug 26, 2019 •

edited

Loading

sirreal commented Aug 27, 2019

sirreal commented Aug 27, 2019

gwwar commented Aug 27, 2019 •

edited

Loading

sirreal left a comment

sirreal Aug 27, 2019

jsnajdr commented Aug 29, 2019

Update WordPress dependencies #35729

Update WordPress dependencies #35729

Conversation

jsnajdr commented Aug 23, 2019 • edited Loading

matticbot commented Aug 23, 2019

matticbot commented Aug 23, 2019 • edited Loading

simison commented Aug 23, 2019

jsnajdr commented Aug 23, 2019

simison commented Aug 23, 2019

simison commented Aug 23, 2019

jsnajdr commented Aug 23, 2019

simison commented Aug 23, 2019 • edited Loading

noahtallen commented Aug 23, 2019

mmtr commented Aug 24, 2019

noahtallen commented Aug 24, 2019

simison Aug 26, 2019

Choose a reason for hiding this comment

sirreal Aug 27, 2019

Choose a reason for hiding this comment

simison Aug 28, 2019

Choose a reason for hiding this comment

sirreal Aug 28, 2019

Choose a reason for hiding this comment

sirreal commented Aug 26, 2019 • edited Loading

sirreal commented Aug 26, 2019

sirreal commented Aug 26, 2019

sirreal commented Aug 26, 2019

jsnajdr commented Aug 26, 2019

jsnajdr commented Aug 26, 2019

jsnajdr commented Aug 26, 2019

simison commented Aug 26, 2019 • edited Loading

sirreal commented Aug 27, 2019

sirreal commented Aug 27, 2019

gwwar commented Aug 27, 2019 • edited Loading

sirreal left a comment

Choose a reason for hiding this comment

sirreal Aug 27, 2019

Choose a reason for hiding this comment

jsnajdr commented Aug 29, 2019

jsnajdr commented Aug 23, 2019 •

edited

Loading

matticbot commented Aug 23, 2019 •

edited

Loading

simison commented Aug 23, 2019 •

edited

Loading

sirreal commented Aug 26, 2019 •

edited

Loading

simison commented Aug 26, 2019 •

edited

Loading

gwwar commented Aug 27, 2019 •

edited

Loading