Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#53192 closed defect (bug) (fixed)

Use hashed/deterministic moduleIDs in Webpack build.

Reported by: peterwilsoncc's profile peterwilsoncc Owned by: gziolo's profile gziolo
Milestone: 5.7.3 Priority: normal
Severity: normal Version: 5.0
Component: Build/Test Tools Keywords: has-patch fixed-major
Focuses: javascript Cc:

Description

The current Webpack configuration uses numeric values for the module IDs. This can result in apparently minor changes to the code causing a large number of files to be rebuilt.

For example [50431] seems to have triggered a complete rebuild of JavaScript files [build50042], the only change in many of the files is the module IDs.

Webpack 4 has an option to generate module IDs based on a hash of the file's path. After some quick testing, I think this will reduce the frequency of complete JavaScript rebuilds.

I've attached a patch but I would love a review and some testing from a few folk more familiar with Webpack.

Note: Webpack 5 deprecates the hashed option in favour of deterministic. Reading the documentation, it looks like the algorithm has changed in addition to the name.

Attachments (3)

53192.diff (711 bytes) - added by peterwilsoncc 4 years ago.
53192-media.diff (344 bytes) - added by gziolo 4 years ago.
53192.2.diff (427 bytes) - added by peterwilsoncc 4 years ago.

Download all attachments as: .zip

Change History (23)

@peterwilsoncc
4 years ago

#1 @desrosj
4 years ago

Related: #51750.

#2 @peterwilsoncc
4 years ago

  • Milestone changed from Awaiting Review to 5.7.3

Putting this on 5.7.3 as I think it will be needed.

I tested this locally by pointing WP Core's packages to the Gutenberg packages:

  • ran a build using the packages on Gutenberg trunk
  • added a random import and pretty meaningless change to the block-editor package
  • rebuilt gutenberg then core packages to see what changed in core
  • most files listed after block-editor in package.json changed in the build, but only the module IDs.
  • repeated the above process with this patch applied to WP Core
  • only the files with code changes showed as modified in the build, module IDs remained the same

It also looks like the compressed files can change by way of module ID without the cache busting value changing in the script-loader file. It seems unlikely but possible this will cause problems if module IDs change without the cache been broken.

@gziolo I know I shared the patch with you via DM in Slack but are you able to post any review notes here so we've got a public log?

Reviews by others with build tools expertise would be most welcome too.

#3 @gziolo
4 years ago

  • Owner set to gziolo
  • Resolution set to fixed
  • Status changed from new to closed

In 50940:

Build: Use hashed/deterministic moduleIDs in webpack config

Props peterwilsoncc, desrosj.
Fixes #53192.

#4 @gziolo
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#5 @peterwilsoncc
4 years ago

  • Keywords fixed-major added

#6 @peterwilsoncc
4 years ago

@gziolo I only noticed the media library used a separate webpack config after this was committed, so it may need changing too. I'm trying to decide whether to make this change on this or a separate ticket but wanted to give you advance warning.

Here's an example of a built media file with numeric module IDs: https://github.com/WordPress/WordPress/blob/7670b7dca84fccc40ec4c3271075046dfc2f6f65/wp-includes/js/media-views.js#L91-L96

The affected files are:

  • /wp-includes/js/media-audiovideo.js
  • /wp-includes/js/media-audiovideo.min.js
  • /wp-includes/js/media-grid.js
  • /wp-includes/js/media-grid.min.js
  • /wp-includes/js/media-models.js
  • /wp-includes/js/media-models.min.js
  • /wp-includes/js/media-views.js
  • /wp-includes/js/media-views.min.js

#7 @gziolo
4 years ago

@peterwilsoncc, good catch. There is another config for media-related files that need to be updated, too. Changes should get applied around these lines:

https://github.com/WordPress/wordpress-develop/blob/7cd5ecd667177828eeb801c74f4592fe744dd2cf/tools/webpack/media.js#L28-L35

@gziolo
4 years ago

#8 @peterwilsoncc
4 years ago

Thanks @gziolo, I will set up a separate ticket but will reach out to you when I've got a patch ready for review.

#9 @gziolo
4 years ago

Shouldn't it be the same ticket, it's a part of one webpack config that uses 2 independent builds. I also included a patch in this ticket that mirrors the production config for packages (and blocks).

#10 @gziolo
4 years ago

In 50984:

Build: Use deterministic module ids in webpack for media

Follow-up for [50940].
Props peterwilsoncc.
See #53192.

#11 @gziolo
4 years ago

https://build.trac.wordpress.org/changeset/50593 - this is how changes for media-related entry points look like with named chunks. It uses file paths even for minified files, but it is exactly the same as for packages, see https://build.trac.wordpress.org/changeset/50549.

Last edited 4 years ago by gziolo (previous) (diff)

#12 @peterwilsoncc
4 years ago

The packages config file has some additional smarts over the media config file to determine if it's the production build, it's within that the hashed module IDs are definded. https://github.com/WordPress/wordpress-develop/blob/187c7fc7252a53b5f355a31a8cb3deadf985f4c7/tools/webpack/packages.js#L365-L372

Changing the new setting from named to hashed will fix this, I'll add a follow up commit in the next few hours.

#13 @peterwilsoncc
4 years ago

In 50985:

Build: Use deterministic module ids in webpack for media.

Used hashed IDs rather the named IDs to reduce file size and remove references to non-existent paths in the final build.

Follow-up for [50940,50984].
Props gziolo, peterwilsoncc.
See #53192.

#14 @peterwilsoncc
4 years ago

[build50594] is the result of [50985] so everything looks to running as expected.

For the 5.7 branch, only [50940] is to be back ported as the packages are expected to changes but the media files are not. Including [50984-50985] will cause unnecessary bloat in the auto-update files.

#15 @peterwilsoncc
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 50990:

Build: Use hashed/deterministic moduleIDs in webpack config

Props peterwilsoncc, desrosj, gziolo.
Merges [50940] in to the 5.7 branch.
Fixes #53192.

#16 @peterwilsoncc
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Per comment 11 I am reopening this to deal with the compressed package.js configured files using the path rather than the hash for module IDs.

As there are different configs, I am wondering if the ( mode === 'development' && env.buildTarget === 'build/' ) condition in package.js needs to change so the optimisation settings take in production mode.

#17 @peterwilsoncc
4 years ago

In 53192.2.diff:

Improves specifying of hashed vs named module IDs for production builds, as mentioned in comment #11 the compressed editor production files are using named IDs, only the uncompressed files were using hashes.

#18 @peterwilsoncc
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 51035:

Build tools: Use hashed module IDs for minified files.

Further improves webpack configuration for editor files to use hashed module IDs in the compressed (*.min.js) production files.

Follow up to [50940].

Props gziolo, peterwilsoncc.
Fixes #53192.

#19 @peterwilsoncc
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

[build50644] shows [51035] is working as expected with compressed production files.

Reopening for merging [51035] in to 5.7.3.

Last edited 4 years ago by peterwilsoncc (previous) (diff)

#20 @peterwilsoncc
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 51036:

Build tools: Use hashed module IDs for minified files.

Further improves webpack configuration for editor files to use hashed module IDs in the compressed (*.min.js) production files.

Follow up to [50940].

Props @gziolo.
Merges [51035] to the 5.7 branch.
Fixes #53192.

Note: See TracTickets for help on using tickets.