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

feat(gatsby): handle search params in resource loader #33209

Merged
merged 17 commits into from
Oct 4, 2021

Conversation

pieh
Copy link
Contributor

@pieh pieh commented Sep 16, 2021

Description

SSR allows making use of query params but our runtime is right now adjusted to make it work well:

  • initial page load will have html that make use of serverData produced with proper query params, but later we fetch page-data without those params and there is potential flicker ( fix(gatsby): fix hydration flicker on initial render of ssr page #33134 doesn't work with it without doing more changes)
  • client side navigation (with gatsby-link) just strips query params and make use of pathname

[ch38142]

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Sep 16, 2021
Comment on lines -175 to -177

// Check for initial page-load redirect
maybeRedirect(window.location.pathname)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was causing some race condition where runtime behaviour is not deterministic. Specifically

window.___webpackCompilationHash = page.page.webpackCompilationHash
might not be executed before this check
// window.___webpackCompilationHash gets set in production-app.js after navigationInit() is called
// So on a direct visit of a page with a browser redirect this check is truthy and thus the codepath is hit
// While the resource actually exists, but only too late
// TODO: This should probably be fixed by setting ___webpackCompilationHash before navigationInit() is called
if (
pageResources.page.webpackCompilationHash !==
window.___webpackCompilationHash
) {

@pieh pieh changed the title [wip] handle search params in resource loader feat(gatsby): handle search params in resource loader Sep 28, 2021
@pieh pieh marked this pull request as ready for review September 28, 2021 14:41
@@ -75,7 +75,7 @@ describe(`Redirects`, () => {
failOnStatusCode: false,
}).waitForRouteChange()

cy.location(`pathname`).should(`equal`, `/redirect-search-hash/`)
cy.location(`pathname`).should(`equal`, `/redirect-search-hash`)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Result of https://github.com/gatsbyjs/gatsby/pull/33209/files#r717559851

Please note that with this change it is actually same as we check client navigation in

it(`should support hash parameter with Link component`, () => {
cy.visit(`/`, {
failOnStatusCode: false,
}).waitForRouteChange()
cy.getTestElement(`redirect-two-anchor`).click().waitForRouteChange()
cy.location(`pathname`).should(`equal`, `/redirect-search-hash`)
cy.location(`hash`).should(`equal`, `#anchor`)
cy.location(`search`).should(`equal`, ``)
})

And same for 2 following changes

@pieh pieh added topic: render-mode Related to Gatsby's different rendering modes and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Sep 28, 2021
Comment on lines -298 to -299

return res.status(404).sendFile(`404.html`, { root })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was breaking some code paths in e2e/prod-runtime tests that were previously not exposed because engines were not generated (this PR adds first SSR pages, so this code path was finally hit in those tests).

Later we have request handler for matchPaths (SSG matchpaths, not handled by engine) and sending 404 was just blocking it. We still have final fallback 404 later in final request handler.

Comment on lines +107 to +112
const maybeQueryString = Object.entries(req.query)
.map(([k, v]) => `${k}=${v}`)
.join(`&`)
if (maybeQueryString) {
searchString = `?${maybeQueryString}`
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can rewrite this into this:

Suggested change
const maybeQueryString = Object.entries(req.query)
.map(([k, v]) => `${k}=${v}`)
.join(`&`)
if (maybeQueryString) {
searchString = `?${maybeQueryString}`
}
searchString = new URL('http://localhost:8000/test?q=234').search

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

The test seems to catch what we want to catch and code looks fine so 👍

@wardpeet wardpeet merged commit 5c22948 into master Oct 4, 2021
@wardpeet wardpeet deleted the query-params-loader branch October 4, 2021 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: render-mode Related to Gatsby's different rendering modes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants