Skip to content
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

[V2] Loading page: avoid seeing infinite reloads #532

Merged
merged 7 commits into from Apr 25, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion packages/pwa-kit-cli/src/ssr/server/build-dev-server.js
Expand Up @@ -153,7 +153,7 @@ export const DevServerMixin = {
if (app.__webpackReady()) {
middleware(req, res, next)
} else {
res.redirect(301, '/__mrt/loading-screen/index.html?loading=1')
this._onAllRequestsBeforeWebpackReady(req, res, next)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

}
})

Expand All @@ -173,6 +173,11 @@ export const DevServerMixin = {
})
},

// eslint-disable-next-line no-unused-vars
_onAllRequestsBeforeWebpackReady(req, res, next) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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) {

res.redirect('/__mrt/loading-screen/index.html?loading=1')
},

_getDevServerHostAndPort(options) {
const split = options.devServerHostName.split(':')
const hostname = split.length === 2 ? split[0] : options.devServerHostName
Expand Down
17 changes: 16 additions & 1 deletion packages/pwa-kit-cli/src/ssr/server/build-dev-server.test.js
Expand Up @@ -130,6 +130,21 @@ describe('DevServer startup', () => {
})
})

describe('DevServer loading page', () => {
test('requesting homepage would temporarily redirect to the loading page, when build is not ready', async () => {
const options = opts()
vmarta marked this conversation as resolved.
Show resolved Hide resolved
const app = NoWebpackDevServerFactory.createApp(options)
app.use('/', DevServerFactory._onAllRequestsBeforeWebpackReady)

return request(app)
.get('/')
.expect(302) // Expecting the 302 temporary redirect (not 301)
.then((response) => {
expect(response.headers.location).toBe('/__mrt/loading-screen/index.html?loading=1')
})
})
})

describe('DevServer request processor support', () => {
const helloWorld = '<div>hello world</div>'

Expand Down Expand Up @@ -230,7 +245,7 @@ describe('DevServer request processor support', () => {
})
})

describe('DevServer startup', () => {
describe('DevServer listening on http/https protocol', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI this is just renaming to eliminate duplicate names.

let server
let originalEnv

Expand Down