Skip to content

Remove the window.AppBoot function #98543

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

Merged
merged 1 commit into from
Feb 3, 2025
Merged

Remove the window.AppBoot function #98543

merged 1 commit into from
Feb 3, 2025

Conversation

jsnajdr
Copy link
Member

@jsnajdr jsnajdr commented Jan 17, 2025

I'm often running into the dreadful "window.AppBoot is not a function error" so I started looking into it.

This PR is the result: let's remove the window.AppBoot function and replace it with a main/boot function that is called directly in the JS code. That's how webpack apps are supposed to be done. Webpack will make sure that the chunks are loaded in the last one, and that the root module which calls main() is run only when all other modules are loaded.

I'm not sure about two things:

  1. In addition to loading the entrypoint, we also preload chunks for the current section (e.g., home or reader). These chunks are loaded after the entrypoint. But the scripts will be executed only after main() was already called, because that call is in the entrypoint chunk. Before this PR, we postponed the main call until all chunks were loaded.
  2. We might need to revisit @arthur791004's Assets: Fix the order of the chunk files #95748 PR that fixed the order of chunks. The code after this PR is more sensitive about the order.

@jsnajdr jsnajdr requested review from sirreal and tyxla January 17, 2025 13:43
@jsnajdr jsnajdr self-assigned this Jan 17, 2025
@jsnajdr jsnajdr requested review from a team as code owners January 17, 2025 13:43
@matticbot matticbot added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 17, 2025
Copy link

github-actions bot commented Jan 17, 2025

@matticbot
Copy link
Contributor

matticbot commented Jan 17, 2025

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

App Entrypoints (~22 bytes removed 📉 [gzipped])

name           parsed_size           gzip_size
entry-stepper        -43 B  (-0.0%)       +8 B  (+0.0%)
entry-login          -43 B  (-0.0%)      -25 B  (-0.0%)
entry-main           -21 B  (-0.0%)       -7 B  (-0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

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.

Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning shared in the description makes sense and so do the changes. I did some smoke testing via calypso.live and everything seems to work as expected.

Copy link
Member

@tyxla tyxla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jsnajdr did you find a reliable way to trigger the window.AppBoot error locally so we can test whether this solves the issue?

@jsnajdr
Copy link
Member Author

jsnajdr commented Jan 20, 2025

did you find a reliable way to trigger the window.AppBoot error locally so we can test whether this solves the issue?

It happens to me all the time locally, whenever there is a hot-update that is not a mere React component handled by React Refresh. In that sense, I can reliably trigger it 🙂

Before merging this, I'd like to change the way how the chunks are now represented in the HTML page. When you go to /read, the scripts are:

<script src="dependencies of entry-main">
<script src="entry-main chunk which includes the call to main()">
<script src="reader chunks to that the /read route can be handled asap">

I'd like to move the Reader chunks to be instead:

<link ref="preload" href="reader chunks" as="script">

because that's what we want: preload the Reader chunks so that when the router handles the /read route, then the await import( 'reader' ) call will have to actually load the chunks into the webpack module registry, but the content will be already downloaded and cached.

The reason why I want preload is that the main() execution is the last thing executed in a sync <script> tag. Don't start loading any other chunks after that. Because that can lead to race conditions. Let's preload the instead.

Cc: @sgomes who probably has existing experience with preload and prefetch and who will surely find this PR interesting 🙂

@jsnajdr jsnajdr requested a review from sgomes January 20, 2025 12:19
@sgomes
Copy link
Contributor

sgomes commented Jan 20, 2025

Hey @jsnajdr! 👋 I can give some advice on preload and friends, sure!

Generally, preload should only be used for things you know for sure you will need, and that are being discovered late for some reason (e.g., a font URL in an external CSS file). That's not really the case here, since the user may never navigate to the reader. Using preload would trigger high priority fetches that would compete for bandwidth with other, more important resources.

One way to mitigate this would be to add fetchpriority="low" to the preload, which will cause the fetch to be scheduled at a lower priority. Both preloads and fetchpriority would be progressive enhancement in this scenario, so everything should still work (albeit less performantly) if either or both are unsupported in the user's browser.

The really really interesting option would be to switch to native modules, though 🙂 If we had access to dynamic import() in the browser, all sorts of cool options would open up, because we'd be free to schedule fetches and manipulate their promises however we saw fit! I realise that's probably too big of a change, though 😅

@jsnajdr
Copy link
Member Author

jsnajdr commented Jan 20, 2025

Generally, preload should only be used for things you know for sure you will need, and that are being discovered late for some reason

This is exactly what we have here. We load the wordpress.com/read, and that is served by entry-main Calypso code that looks like this:

page( '/read', async () => {
  await import( 'reader' );
  next();
}, ... );
page.start();

We know that import( 'reader' ) is going to be called very soon, and we should preload the chunks.

Until now we've been loading the reader chunks directly and synchronously, by adding <script> tags that load them. And that delays the window.AppBoot call. Now when the app would start right when the entry-main chunk is executed, there is a weird kind of race conditon where the app is already running, and at the same the <script> tags for reader are still executing. I'm not 100% sure that is safe, so I'd like to switch to preloading.

The really really interesting option would be to switch to native modules, though

Oh, that would be very nice! And easier to implement than in Gutenberg, which has too many compatibility constraints.

@sgomes
Copy link
Contributor

sgomes commented Jan 20, 2025

This is exactly what we have here. We load the wordpress.com/read, and that is served by entry-main Calypso code

Oh, sorry, I misunderstood what you meant! I didn't realise that you were referring to two stages within the same navigation.

Yes, in that case, preload does seem appropriate, and it may even be a good idea to set fetchpriority="high". I'd test both fetchpriority="high" and no fetchpriority to see which one performs better, as it's heavily dependent on what else might be going on.

Once again, preload and fetchpriority should both serve as progressive enhancement and not break browsers without support 👍

Oh, that would be very nice! And easier to implement than in Gutenberg, which has too many compatibility constraints.

The biggest constraint tends to be the same-origin policy, but I don't think that's an issue in this case.

@jsnajdr
Copy link
Member Author

jsnajdr commented Jan 23, 2025

Once again, preload and fetchpriority should both serve as progressive enhancement and not break browsers without support 👍

@sgomes Yes, they are a mere optimization / progressive enhancement. If the preload didn't happen at all, then webpack runtime would load all the chunks anyway. They just won't be preloaded.

I'm implementing this in #98799.

@jsnajdr jsnajdr force-pushed the remove/window-appboot branch from ccd1ac4 to fcfbf48 Compare February 3, 2025 08:08
@matticbot
Copy link
Contributor

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 remove/window-appboot on your sandbox.

@jsnajdr
Copy link
Member Author

jsnajdr commented Feb 3, 2025

All the prererequisites for this PR are done:

Now it's time to merge also this one 🙂

@jsnajdr jsnajdr merged commit 4d3d94f into trunk Feb 3, 2025
13 checks passed
@jsnajdr jsnajdr deleted the remove/window-appboot branch February 3, 2025 08:30
@github-actions 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
@tyxla
Copy link
Member

tyxla commented Feb 5, 2025

Awesome!

Let's p2 about this one, @jsnajdr! Just so folks are aware of what we did, and if anyone hits into similar issues as they change branches, they would report to us.

JessBoctor pushed a commit that referenced this pull request Mar 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants