-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
Minimal change to avoid canonical redirect #15180
Conversation
@@ -0,0 +1,3 @@ | |||
exports.onClientEntry = () => { | |||
window.browserLoc.pathname = window.pagePath |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, this could be integrated in https://github.com/wardpeet/gatsby-plugin-static-site
@@ -28,6 +28,12 @@ setApiRunnerForLoader(apiRunner) | |||
|
|||
navigationInit() | |||
|
|||
window.browserLoc = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is needed to decouple the expected location with the real location, and be able to modify it in onClientEntry
calls.
Is there anything missing I can do to get this PR considered? |
Hey @xavivar, thank you for opening this pull request! Unfortunately, the proposed change is not going to be merged. This is a fairly unusual usecase, and while we'd love to support every usecase for everyone we don't have the capacity to do that right now. It may seem like a small change but any time we expose apis publicly like this it means we're committed to supporting them, which can lead to problems when we change them later on. I'll see if I can get it working in https://www.gatsbyjs.org/packages/@wardpeet/gatsby-plugin-static-site. Another approach could be to install https://www.gatsbyjs.org/packages/gatsby-plugin-no-javascript/ to remove all of Gatsby's JS. This uses existing apis so should continue to work with future Gatsby versions. Thanks again, and we look forward to seeing more PRs from you in the future! 💪💜 |
Hi @wardpeet , I'd love to have this in the static plugin, but without a way to override (and accesing Regarding the no-javascript plugin, does that mean that we can't use react either, right? |
Hi @xavivars. I've put some effort in wardpeet/gatsby-plugin-static-site#4 to get your solution inside a plugin. This should work.
yeah this just enables ssr, nothing else. |
Thank you so much for that work! I'm trying to better understand the changes, mainly the ones about the loader. What it's doing is using pagePath to download all json files etc, instead of browser's location, right? And I didn't know about replace's usage in navigate, but it's actually pretty smart! |
⬆️ all that you said is true! I've noticed that we use replace to fix these wrongly matched paths. So we don't do any redirect. That fixes the redirect part. The loader overrides are like you said, if we get an url that doesn't look like pagePath we override it. With this override, we send the original path to the pageDataLoader, so our javascript gets loaded. If I didn't do that, I would get a 404 on our page-data.json, and our js won't work. |
Description
This PR introduces a way to override browser location that is used to check canonical redirects, just before onClientEntry so plugins or gatsby-browser.js can change the behavior.
This is a simplified version of #15173, but will also allow avoiding redirects when a page loads, in case the paths are different when generated and when displayed.
This used together with https://github.com/wardpeet/gatsby-plugin-static-site allows to use gatsby as a static page generator instead of a static site generator.
Related Issues
This PR fixes #4337, and superseeds #15173