Assets: Fix the order of the chunk files (2nd try) #96100

arthur791004 · 2024-11-06T08:26:42Z

Related to #95748

Proposed Changes

We merged Assets: Fix the order of the chunk files #95748 before but it seemed weird that the chunk files became empty on production...

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

github-actions · 2024-11-06T08:26:56Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-131721

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-131721&env=jetpack

Automattic for Agencies live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-131721&env=a8c-for-agencies

arthur791004 · 2024-11-06T08:28:24Z

I'll come back to this when I have time.

matticbot · 2025-01-19T13:19:16Z

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

notifications

To test WordPress.com changes, run install-plugin.sh $pluginSlug fix/webpack-chunk-files-order on your sandbox.

matticbot · 2025-01-19T13:21:29Z

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

jsnajdr · 2025-01-23T12:26:54Z

Hi @arthur791004 👋 I'd like to merge this patch again. To do that responsibly, I'd like to know some details why exactly it failed the first time, on Nov 6 2024. In this comment you say that production chunks were empty. Do you remember anything beyond that? I tried to find some Slack thread about it, but didn't find anything.

I'd like to avoid discovering exactly the same problem again and reverting again, without learning anything new. The second try should be in some sense better, more informed than the first one.

arthur791004 · 2025-01-23T12:32:11Z

Do you remember anything beyond that? I tried to find some Slack thread about it, but didn't find anything.

I checked the response html after the deployment and found that the chunk files were not listed. It didn't break anything, but it caused the preload not to work.

jsnajdr · 2025-01-23T12:46:03Z

I checked the response html after the deployment and found that the chunk files were not listed.

Thanks, this is already useful info. I understand it as:

it's not that the chunk files would be empty, i.e., the HTTP responses, but the <script> tags were not included in the HTML document
the <script> tags for the entrypoint were there, so the app booted up and worked. It's only the preloads for the initial section (e.g., the reader chunk for the /read route) that were missing.

Is that right?

arthur791004 · 2025-01-23T12:48:04Z

Yes, exactly!

jsnajdr · 2025-01-23T13:22:38Z

I can reproduce this on calypso.live. The preloads for the section JS chunks are missing. Locally, in dev mode on calypso.localhost they are there.

jsnajdr · 2025-01-23T18:59:28Z

build-tools/webpack/assets-writer-plugin.js

@@ -82,10 +82,8 @@ Object.assign( AssetsWriter.prototype, {

 			statsToOutput.chunks = stats.chunks.map( ( chunk ) =>
 				Object.assign( {}, chunk, {
+					chunks: stats.namedChunkGroups[ chunk.id ]?.chunks,


This line causes the problem with missing chunks in production. The chunk.id is a chunk ID, but stats.namedChunkGroups uses chunk names as keys. In development they are both identical (we use chunkIds: 'named') but in production the chunk IDs are numbers.

I fixed this by a radical refactoring of the assets.json file format 🙂 It turns out that everything the app needs to serve the right assets is in the stats.namedChunkGroups object and in the assets field of this object's entries. For each chunk group, assets is a combined array of all assets from all chunks that comprise a chunk group. We don't need to look at chunks of the chunk group and then find all individual chunks one by one and read their assets. It's all already prepared by webpack at one place.

In the server code that reads the assets.json file, I also unified the two functions getFilesForEntrypoint and getFilesForChunk. Because entrypoint is just a special type of a chunk group. We only need one function, getFilesForChunkGroup.

jsnajdr · 2025-01-24T15:45:19Z

build-tools/webpack/assets-writer-plugin.js

+				}
+
+				return asset.info.hotModuleReplacement || asset.info.development;
+			}


This fixes one of possible causes of the "window.AppBoot is not a function" error. In development mode, we were writing the *.hot-update.* assets to the assets.json file, and the dev server was probably serving them as one of the scripts in the initial HTML page. That's wrong, hot updates are supposed to be sent only to a live page without reload. Once you reload, you'll load the updated asset.

See how html-webpack-plugin does exactly the same filtering of assets that will be included on the HTML page: https://github.com/jantimon/html-webpack-plugin/blob/7299866767d7256c65e1d11daaed83426b6d74aa/index.js#L558-L564

I discovered this by accident when having the assets.json file open in the editor and suddenly noticing suspicious entries there:

Nice find 👍

jsnajdr · 2025-01-24T15:51:53Z

@tyxla @arthur791004 I believe this PR is now fixed and ready. The assets.json file now contains only the information that is needed and nothing else. In the manifests field, there are inline scripts that are supposed to be inline in the HTML page. And the assets field contains list of asset names for each chunk group: either an entrypoint, or an async import.

The asset writer code has been incrementally patched for years, since webpack 1 all the way to webpack 5, and accumulated some noise. Now it's cleaned up again.

How to test:
Load /read or /home on calypso.live and verify that:

the entry-main scripts are loaded at the bottom of the <body>
the reader or home scripts are preloaded in the <head>
all the CSS for both entry-main and reader is loaded fully in <head>, because the initial HTML can in theory contain server-rendered Reader markup that needs the Reader styles from the very beginning

tyxla

Tests as expected 👍

I think we should try it out.

Also left a few minor questions, nothing blocking though.

Let's make sure to test thoroughly once on production.

tyxla · 2025-01-27T11:32:51Z

bin/loader-stats.js

-#!/usr/bin/env node
-const path = require( 'path' );
-const stats = require( path.join( __dirname, '..', 'build', 'assets.json' ) );
-const gzipSize = require( 'gzip-size' );


Ah, we can actually remove the gzip-size dependency now 👍

Someone here has a well trained detector of removable code 👍 Pushed a commit that removes gzip-size from the NPM dependencies.

tyxla · 2025-01-27T11:58:19Z

bin/loader-stats.js

@@ -1,80 +0,0 @@
-#!/usr/bin/env node


Is this script completely unused? Was it ever used for anything beyond debugging during major webpack upgrades?

Yes, we discussed this in the try-1 version of this PR: #95748 (comment)

The script has been completely replaced by ICFY.

tyxla · 2025-01-27T12:13:31Z

build-tools/webpack/assets-writer-plugin.js

+					return false;
+				}
+
+				return asset.info.hotModuleReplacement || asset.info.development;


Wasn't able to see a single asset where development is true. Is this intended to be for source maps? If we're not sure, should we keep the pre-existing *.map check?

Yes, development: true is a flag that source maps have. It's reliable, html-webpack-plugin also uses it, for the same job as we do here: figure out which compilation assets and how to embed into the app's HTML document.

Here is a webpack source reference where the source maps plugin adds the flag: https://github.com/webpack/webpack/blob/3919c844eca394d73ca930e4fc5506fb86e2b094/lib/SourceMapDevToolPlugin.js#L547-L550

tyxla · 2025-01-27T12:23:28Z

client/server/pages/test/index.js

+					'entry-domains-landing': assetsList.map( ( asset ) =>
+						asset.replace( 'entry-main', 'entry-domains-landing' )
+					),
+					'entry-gutenboarding': assetsList.map( ( asset ) =>


Oh my, this is long gone 😅 let's remove it.

tyxla · 2025-01-27T12:26:28Z

build-tools/webpack/assets-writer-plugin.js

+				}
+
+				return asset.info.hotModuleReplacement || asset.info.development;
+			}


Nice find 👍

arthur791004 self-assigned this Nov 6, 2024

jsnajdr force-pushed the fix/webpack-chunk-files-order branch from 05fe9bb to 21b5627 Compare January 19, 2025 13:11

jsnajdr force-pushed the fix/webpack-chunk-files-order branch from 21b5627 to f279841 Compare January 23, 2025 12:51

jsnajdr reviewed Jan 23, 2025

View reviewed changes

jsnajdr force-pushed the fix/webpack-chunk-files-order branch from ba58ad8 to f561b74 Compare January 24, 2025 12:14

jsnajdr reviewed Jan 24, 2025

View reviewed changes

jsnajdr requested a review from tyxla January 24, 2025 15:47

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jan 24, 2025

tyxla approved these changes Jan 27, 2025

View reviewed changes

arthur791004 and others added 9 commits February 1, 2025 11:14

Assets: Fix the order of the chunk files

69d29ac

Get rid of fallback

503863b

Fix tests

dac61e9

Update copy

942b1db

Simplify the info in the assets.json file

e55e971

Unify getFilesForEntrypoint and getFilesForChunk

22a0c34

Fix server unit tests

65427f9

Exclude hot updates and sourcemaps from assets.json served by server

a4961b3

Remove gzip-size npm dependency

c0de2a3

jsnajdr force-pushed the fix/webpack-chunk-files-order branch from 5187f8a to c0de2a3 Compare February 1, 2025 13:23

jsnajdr merged commit c0cb77b into trunk Feb 3, 2025
13 checks passed

jsnajdr deleted the fix/webpack-chunk-files-order branch February 3, 2025 07:25

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 3, 2025

jsnajdr mentioned this pull request Feb 3, 2025

Remove the window.AppBoot function #98543

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assets: Fix the order of the chunk files (2nd try) #96100

Assets: Fix the order of the chunk files (2nd try) #96100

arthur791004 commented Nov 6, 2024

github-actions bot commented Nov 6, 2024 •

edited by matticbot

Loading

arthur791004 commented Nov 6, 2024

matticbot commented Jan 19, 2025 •

edited

Loading

matticbot commented Jan 19, 2025

jsnajdr commented Jan 23, 2025

arthur791004 commented Jan 23, 2025

jsnajdr commented Jan 23, 2025

arthur791004 commented Jan 23, 2025

jsnajdr commented Jan 23, 2025

jsnajdr Jan 23, 2025

jsnajdr Jan 24, 2025

jsnajdr Jan 24, 2025 •

edited

Loading

tyxla Jan 27, 2025

jsnajdr commented Jan 24, 2025

tyxla left a comment

tyxla Jan 27, 2025

jsnajdr Feb 1, 2025

tyxla Jan 27, 2025

jsnajdr Feb 1, 2025

tyxla Jan 27, 2025

jsnajdr Feb 1, 2025

tyxla Jan 27, 2025

tyxla Jan 27, 2025

Assets: Fix the order of the chunk files (2nd try) #96100

Assets: Fix the order of the chunk files (2nd try) #96100

Conversation

arthur791004 commented Nov 6, 2024

Proposed Changes

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

github-actions bot commented Nov 6, 2024 • edited by matticbot Loading

arthur791004 commented Nov 6, 2024

matticbot commented Jan 19, 2025 • edited Loading

matticbot commented Jan 19, 2025

jsnajdr commented Jan 23, 2025

arthur791004 commented Jan 23, 2025

jsnajdr commented Jan 23, 2025

arthur791004 commented Jan 23, 2025

jsnajdr commented Jan 23, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsnajdr Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsnajdr commented Jan 24, 2025

tyxla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 6, 2024 •

edited by matticbot

Loading

matticbot commented Jan 19, 2025 •

edited

Loading

jsnajdr Jan 24, 2025 •

edited

Loading