feat(core): rework client modules lifecycles, officially make API public

[Josh-Cena]

Breaking changes

Client module lifecycle onRouteUpdateDelayed was removed. You can restore a similar feature using the new lifecycles + setTimeout (see our NProgress integration as an example).

The existing onRouteUpdate lifecycle will now also fire on first browser page render (hydration), similarly to the new onRouteDidUpdate

Documentation: https://deploy-preview-6732--docusaurus-2.netlify.app/docs/advanced/client#client-module-lifecycles

More details here: #3399 (comment)

Motivation

Fix #3399. Client modules suffer from the issue of firing too early—before React even renders. In this case, people who use client modules to modify DOM on every page, or simply reading from it (like the route announcer in #7074) will not be able to do so without using a timeout hack.

This PR also addresses a long-standing issue of hash links not working in development. The problem is that the scroll-to-anchor happens in shouldComponentUpdate, so if there's no pre-rendered HTML, there will be no DOM element to read from. This is solved through deferring it to after the initial render as well.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

I added a dogfooding client module. When switching between routes, note how the lifecycle now logs the new title instead of the old one:

[netlify]

✅ [V2]

Name	Link
🔨 Latest commit	c244bb1
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/626bdb0c4a200000090e9002
😎 Deploy Preview	https://deploy-preview-6732--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 changes in this PR:

Category	Score
🟠 Performance	57
🟢 Accessibility	100
🟠 Best practices	83
🟢 SEO	100
🟢 PWA	90

Lighthouse ran on https://deploy-preview-6732--docusaurus-2.netlify.app/

[github-actions]

Size Change: +399 B (0%)

Total Size: 803 kB

Filename	Size	Change
website/build/assets/css/styles.********.css	107 kB	+168 B (0%)
website/build/assets/js/main.********.js	607 kB	+229 B (0%)

ℹ️ View Unchanged

Filename	Size	Change
website/.docusaurus/globalData.json	50.1 kB	+2 B (0%)
website/build/index.html	38.8 kB	0 B

_{compressed-size-action}

[slorber]

Looks like a useful change to do that 👍

See comments: I'd rather really use useEffect today as most usage is analytics IMHO, should be non-blocking (perf) and should be fail-safe. Maybe the lifecycle should somehow respect React terminology? Like onRouteUpdateEffect + eventually onRouteUpdateLayoutEffect?

Also worth migrating our GA plugins to this new lifecycle?

Docs? or do we dogfood this a bit before writing it?
(considering it impacts the plugin authors we should rather get it right the fist time)

[Josh-Cena]

useEffect or useLayoutEffect?

If we are talking about firing analytics, then it doesn't really matter, because the effect doesn't affect UI anyways, it just leaves.

If we are talking about manipulating DOM (for example, someone has presented a weird use-case to me where they have an external script that needs to manipulate a particular DOM element on every page transition), then it certainly needs to be synchronous. I can't think of a compelling case where the lifecycle would block render in an unacceptable way.

[slorber]

If we are talking about firing analytics, then it doesn't really matter, because the effect doesn't affect UI anyways, it just leaves.

Actually it can affect UI:

• the analytics SDK could fail, leading to the page transition never completing
• the analytics SDK could be slow, serialize synchronously a lot of JSON or whatever, leading to a delayed page transition UX

That's why I think firing in useEffect is a better default.

This is also why the React team created useEffect and useLayoutEffect and we use useEffect in most cases by default. They could have given us useLayoutEffect only and told us to use setTimeout but they didn't for good reasons: help us do the right thing by default. We should do the same for our users.

someone has presented a weird use-case to me where they have an external script that needs to manipulate a particular DOM element on every page transition

Then we need to understand better this use-case

[Josh-Cena]

Then we need to understand better this use-case

I remember it was attaching event listeners.

import ExecutionEnvironment from '@docusaurus/ExecutionEnvironment';

export default (function () {
  if (!ExecutionEnvironment.canUseDOM) {
    return null;
  }

  return {
    onRouteUpdate({location}) {
      if (location.pathname.startsWith('/acs/')) {
        const labels = document.getElementsByTagName("label");
        for (let a = 0; a < labels.length; a++)
          labels[a].addEventListener("click", (e) => e.target.classList.add("foo"));
        document.querySelector('input,textarea').attr('autocomplete', 'off');
      }
    },
  };
})();

More complicated than that though, because they are using some 3rd-party script that they don't really want to refactor into their React app, so they just wanted a standalone <script>. However, this is the only way to make the script re-execute on every page.

[Josh-Cena]

@slorber I implemented what you had in mind in a4e16b9. Tell me what you think and if that's the current direction we should go in.

I realized because useLayoutEffect still fires after React renders, even if we put onRouteUpdate in useLayoutEffect, the lifecycle is able to get the latest DOM on the new page. Therefore I think it's not necessary to make another lifecycle. If we are to continue supporting this lifecycle firing before React re-render, we have to put it in shouldComponentUpdate.

[slorber]

LGTM 👍

[Josh-Cena]

Also, don't we want ga/gtag plugin events to trigger only after React renders the next page?

Migrated to onRouteDidUpdate. Impact should be minimal. There's a nuance: onRouteUpdate only fires when route changes, but onRouteDidUpdate would fire on initial render as well. Do you think that's a good thing, or it doesn't really matter?

[slorber]

There's a nuance: onRouteUpdate only fires when route changes, but onRouteDidUpdate would fire on initial render as well. Do you think that's a good thing, or it doesn't really matter?

😭 I don't know 😅

It looks like it does have an impact and lead to duplicate events being send on site first load 😓

Before/prod:

After:

Maybe both lifecycles should never fire on the very first render when previousLocation is null?

I feel like having this difference between the 2 lifecycles will be confusing for users, and we should rather make the best choice now because this change later can also be painful for the plugin ecosystem.

Note that client module authors can still add some inline JS that will be executed immediately after load:

if (ExecutionEnvironment.canUseDOM) {
  console.log("on first load, document title is " + document.title);
}

Note that Gatsby seems to fire both lifecycles on first load:

• https://www.gatsbyjs.com/docs/reference/config-files/gatsby-browser/#onPreRouteUpdate
• https://www.gatsbyjs.com/docs/reference/config-files/gatsby-browser/#onRouteUpdate
• https://www.gatsbyjs.com/docs/reference/config-files/gatsby-browser/#onInitialClientRender

[Josh-Cena]

We can fire it as frequently as we want, and ask client module authors to filter out unwanted triggers. For example, here we can add a simple if (previousLocation === null). The only problem is I don't really know which is better off for the user: is it useful for analytics to know the first page the user is on, or should it only track navigations?

[slorber]

We can fire it as frequently as we want, and ask client module authors to filter out unwanted triggers. For example, here we can add a simple if (previousLocation === null). The only problem is I don't really know which is better off for the user: is it useful for analytics to know the first page the user is on, or should it only track navigations?

I guess it's more powerful to let the users figure out the filtering on their side in case duplicate events are fired on initial load.

Some analytics may send events automatically to their endpoint on js load, while others need to be triggered manually with a JS API. Giving the opportunity to trigger this API seems better to me 👍

Now we should probably make both lifecycle consistent, and it's not very clear to me when/where onRouteUpdate should be fired in the app. It could be something like this?

      if (previousLocation === null) {
        const cleanup = dispatchLifecycleAction('onRouteUpdate', {
          previousLocation,
          location,
        });
        cleanup();
      }
      
      dispatchLifecycleAction('onRouteDidUpdate', {previousLocation, location});

[slorber]

I suggest we do like Gatsby: both lifecycles fire on initial render.

Firing from PendingNavigation constructor seems ok 🤷‍♂️ not exactly like my suggestion above but it probably doesn't matter much

https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/cache-dir/navigation.js#L217

Now we need to ignore GA calls when !previousLocation (first render) because it seems to already emit a pageview event on load

[Josh-Cena]

Okay, why not Hmm, the lifecycle is fired from a ref, so during constructor phase the ref hasn't attached yet. I'll just expose the imperative handle directly.

[slorber]

seems good now 👍

just need to filter first call on gtag/google-analytics plugins

[Josh-Cena]

This check actually isn't necessary before either...

[slorber]

tested locally, analytics work as before 👍

great 🎉

[axe312ger]

Cool!