[V2] Loading page: avoid seeing infinite reloads

[vmarta]

Description

Ticket: W-11025868

It's possible that the loading page to be reloading infinitely. At a glance, the issue seems to be the loading page infinitely reloading itself. But what happens is actually it redirects to the homepage, which then redirect you back to the loading page → infinite loop

Bug in action:
https://user-images.githubusercontent.com/847300/164569459-1cf72af3-9c18-4acb-ae10-1f6c0bfb0de8.mp4

There are 2 possible redirects between the loading page and homepage:

• loading page → homepage: this happens when the webpack build is ready
• homepage → loading page: this happens when the build is NOT ready

If you somehow trigger the 2nd redirect above, the browser would cache the redirect to the loading page. Then afterwards it's possible for you to run into the infinite loop.

The fix is by making sure that the redirect is using 302 temporary redirect (and not the permanent 301).

Types of Changes

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Documentation update
• Breaking change (could cause existing functionality to not work as expected)
• Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

• Removing a public function or component or prop
• Adding a required argument to a function
• Changing the data type of a function parameter or return value
• Adding a new peer dependency to package.json

Changes

• Converted the redirect from homepage to loading page to use 302
• Added a test to make sure it's 302 redirect

How to Test-Drive This PR

1. npm ci at the root
2. cd /packages/template-typescript-minimal
3. npm start
4. When the loading page shows up in your browser, quickly navigate to the homepage by typing localhost:3000 into the address bar.
5. You'll get redirected back to the loading page, since the webpack build is NOT ready yet
6. Now that the build is ready, you'll get redirected to the homepage
7. Verify that you are now at the homepage (and not seeing the infinite loop)

Checklists

General

• Changes are covered by test cases
• CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

• There are no changes to UI

or...

• Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
• Changes comply with WCAG 2.0 guidelines levels A and AA
• Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

• Changes include a UI text update in the Retail React App (which requires translation)

[vmarta]

This redirect now uses the default behaviour, which is using 302 http status.

[vmarta]

A bit of refactoring, so that it can be called for testing purpose.

[bendvc]

I'm guessing the reason for refactoring is so you don't have use some kind of module mock for the webpackHotServerMiddleware correct?

I think your approach is good but _useWebpackHotServerMiddleware isn't really representative of what is actually happening in that function. I believe if we were to come up with a name it would be something along the lines of useLoadingPageMiddleware because what is being passed to app.use is in fact a middle ware definition which in turn uses another middleware (WebpackHotServerMiddleware).

We could think of this as a naming issue, but I think there is a simpler solution and that would be to create a function called _getHotServerMiddleware(app) (public or private, i'm not sure). It might look like this:

getHotServerMiddleware(app): {
    return webpackHotServerMiddleware(app.__compiler)
}

You'd use it in your app, replacing:

const middleware = webpackHotServerMiddleware(app.__compiler)

with

const middleware = getHotServerMiddleware(app)

Now inside your test you can define that function on the NoWebpackDevServerFactory like:

const NoWebpackDevServerFactory = {
    ...DevServerFactory,
    ...{
        addSSRRenderer() {},
        addSDKInternalHandlers() {},
        getRequestProcessor() {},
        getHotServerMiddleware() {} <-- new
    }
}

[vmarta]

FYI this is just renaming to eliminate duplicate names.

[vmarta]

@bendvc I see now how the previously the function _useWebpackHotServerMiddleware isn't accurate. How about this instead?

What I actually wanted is to extract out the piece that does the redirect for those requests before webpack build is ready.

[olibrook]

Looks good to me. Can we change the name to describe what this fn does though please? It doesn't do anything with "all requests"!

Suggested change

	_onAllRequestsBeforeWebpackReady(req, res, next) {
	_redirectToLoadingScreen(req, res, next) {