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

Minimal change to avoid canonical redirect #15180

Closed
wants to merge 3 commits into from
Closed

Minimal change to avoid canonical redirect #15180

wants to merge 3 commits into from

Conversation

xavivars
Copy link
Contributor

@xavivars xavivars commented Jun 27, 2019

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

@@ -0,0 +1,3 @@
exports.onClientEntry = () => {
window.browserLoc.pathname = window.pagePath
Copy link
Contributor Author

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 = {
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 is needed to decouple the expected location with the real location, and be able to modify it in onClientEntry calls.

@xavivars
Copy link
Contributor Author

xavivars commented Jul 2, 2019

Is there anything missing I can do to get this PR considered?

@wardpeet
Copy link
Contributor

wardpeet commented Jul 3, 2019

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! 💪💜

@wardpeet wardpeet closed this Jul 3, 2019
@xavivars
Copy link
Contributor Author

xavivars commented Jul 3, 2019

Hi @wardpeet , I'd love to have this in the static plugin, but without a way to override (and accesing window.location directly, I don't see how that can happen. This PR was created as an alternative to something way more invasive like #15173

Regarding the no-javascript plugin, does that mean that we can't use react either, right?

@wardpeet
Copy link
Contributor

wardpeet commented Jul 3, 2019

Hi @xavivars. I've put some effort in wardpeet/gatsby-plugin-static-site#4 to get your solution inside a plugin. This should work.

Regarding the no-javascript plugin, does that mean that we can't use react either, right?

yeah this just enables ssr, nothing else.

@xavivars
Copy link
Contributor Author

xavivars commented Jul 3, 2019

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!

@wardpeet
Copy link
Contributor

wardpeet commented Jul 3, 2019

⬆️ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disable Client-Side Routing?
2 participants