refactor(pwa): remove reloadPopup option in favor of swizzling

[Josh-Cena]

Pre-flight checklist

• I have read the Contributing Guidelines on pull requests.
• If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.
• If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Breaking change

The reloadPopup option of the PWA plugin is removed. If you are using a custom component, consider swizzling @theme/PwaReloadPopup instead. If you set it to false, consider returning null from your component.

Motivation

The dynamic import used to import the reload popup component has caused problems with server-side rendering due to how Babel/SWC compiles it. See #3742 and #7329 for some struggles. I propose to simply remove this option, because we can leverage swizzling to achieve the same effect.

Test Plan

Test links

Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/

Related issues/PRs

[netlify]

✅ [V2]

Name	Link
🔨 Latest commit	d71b10f
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/6290c1d58454ef0008fcf207
😎 Deploy Preview	https://deploy-preview-7422--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[github-actions]

⚡️ Lighthouse report for the deploy preview of this PR

URL	Performance	Accessibility	Best Practices	SEO	PWA	Report
/	🟠 77	🟢 100	🟢 100	🟢 100	🟢 90	Report
/docs/installation	🟠 86	🟢 99	🟢 100	🟢 100	🟢 90	Report

[github-actions]

Size Change: -98 B (0%)

Total Size: 796 kB

Filename	Size	Change
website/build/assets/js/main.********.js	598 kB	-98 B (0%)

ℹ️ View Unchanged

Filename	Size
website/.docusaurus/globalData.json	52.3 kB
website/build/assets/css/styles.********.css	106 kB
website/build/index.html	38.9 kB

_{compressed-size-action}

[slorber]

That looks fine 👍

However we also used dynamic imports so that the reload popup does not need to be loaded eagerly. Implemented initially in #2205

Afaik we had 2 dynamic imports

const renderReloadPopup = (await import('./renderReloadPopup')).default;

  const {default: ReloadPopup} = await import(process.env.PWA_RELOAD_POPUP!);

The dynamic import used to import the reload popup component has caused problems with server-side rendering due to how Babel/SWC compiles it. See #3742 and #7329 for some struggles. I propose to simply remove this option, because we can leverage swizzling to achieve the same effect.

It's still not clear to me why this is a problem, and which one is the actual problem (both?). The issues didn't really help me. Can you explain? If this is a Babel problem, how does it compile it exactly and how should it compile it?

Can we keep the lazy behavior and still fix the issue?

I'm ok to simplify if it fixes things, but I also like to understand the issue if possible 🤪

[slorber]

we'd rather add a forbidden message here otherwise users with custom popups may miss this

[Josh-Cena]

Yes yes, we can lazy load it. The only problem is we can't use a dynamic string. I think a static one will be fine (that's how registry.js looks like)

On my phone rn, I can write a more detailed explanation when I get to my keyboard

[Josh-Cena]

Here's how babel-plugin-dynamic-import-node is working. require(s) prevents it from being statically extracted. As direct comparison:

    // 2.3.0
-   const a = await Promise.resolve().then(() => _interopRequireWildcard(require(`${process.env.FOO}`)));
    // 2.3.3
+   const a = await Promise.resolve(`${process.env.FOO}`).then(s => _interopRequireWildcard(require(s)));

So the solution is we should not use process.env but statically use "@theme/ReloadPopup". This way, it will be compiled to

const a = await Promise.resolve().then(() =>
    _interopRequireWildcard(require("foo"))
  );

Because it doesn't need to evaluate anything. The behavior that the path string is evaluated synchronously instead of in the then callback is introduced in airbnb/babel-plugin-dynamic-import-node#85; later, to be more Webpack-compatible for the static string case, airbnb/babel-plugin-dynamic-import-node#89 was made.

[slorber]

Thanks 👍 LGTM