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

fix: make redirection work when page is not existing in previous version #1350

Merged
merged 6 commits into from
Jun 21, 2023

Conversation

12rambau
Copy link
Collaborator

@12rambau 12rambau commented Jun 14, 2023

Fix #1343

EDIT:

Ok I got it, in short return false is one way to prevent many things but is not robust on every browser. Per se a listener should not return anything. It's an old jquery way of doing things that as been implemented by many browsers. It does the following:

  • prevent Default
  • stop Propagation
  • Stops callback execution and returns immediately when called.

To make it more robust I replaced the return statement by the only thing we need: event.preventDefault

Note that it will not work if you use a right-click as you select "open link in a new tab" which will not trigger any redirection.

resources:
https://stackoverflow.com/questions/10219073/html-how-to-prevent-the-browser-from-opening-the-link-specified-in-href
https://web.archive.org/web/20140724160732/http://fuelyourcoding.com/jquery-events-stop-misusing-return-false/

@12rambau
Copy link
Collaborator Author

don't ask me why but adding an extra console.log in the fetch method is making it work.

@drammock
Copy link
Collaborator

don't ask me why but adding an extra console.log in the fetch method is making it work.

I thought I noticed that behavior last week on the other PR but somehow didn't believe / didn't figure out that it really was the console.log that made the difference. 👀

@drammock
Copy link
Collaborator

does not work on PR build, but that's still because of CORS:

Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://pydata-sphinx-theme.readthedocs.io/en/latest/user_guide/install.html. (Reason: CORS header ‘Access-Control-Allow-Origin’ missing). Status code: 200.

@12rambau 12rambau marked this pull request as ready for review June 20, 2023 19:09
@12rambau
Copy link
Collaborator Author

@drammock you've been too fast, I updated the main comment and I think I found the main reason. I don't see CORS error on my system with the latest build (Firefox on a Win 11) is it still an issue for you ?

@drammock
Copy link
Collaborator

@drammock you've been too fast, I updated the main comment and I think I found the main reason. I don't see CORS error on my system with the latest build (Firefox on a Win 11) is it still an issue for you ?

yes I still see the issue. If I go here: https://pydata-sphinx-theme--1350.org.readthedocs.build/en/1350/user_guide/install.html and use version switcher to go to latest or stable, I still end up at the homepage of latest or stable, not the installation page. And here's the relevant bit of the browser log:

redirect

@12rambau
Copy link
Collaborator Author

ok, that's actually expected as it's going from the "pydata-sphinx-theme--1350.org.readthedocs.build" domain to "pydata-sphinx-theme.readthedocs.io" so fetch is raising an error 200 and redirecting, which is what we want rignt ?
As long as PR builds and main site will be on different domain we won't be able to find any workaround here but you can test it locally by starting a server (Python or stb).

@drammock
Copy link
Collaborator

ok, that's actually expected as it's going from the "pydata-sphinx-theme--1350.org.readthedocs.build" domain to "pydata-sphinx-theme.readthedocs.io" so fetch is raising an error 200 and redirecting, which is what we want rignt ?

Yes I know it's expected (at least it's not what you were hoping to fix here). But it's not what we want; we only want to redirect when the page doesn't exist so in an ideal world we would be able to set the CORS origin policy on the readthedocs.io server to allow HEAD requests coming from the PR build's readthedocs.build domain.

side note: it's not fetch() that raises the error, it's handled by the browser. There are browser command-line flags and also browser plugins that will disable the CORS checking (but the effect is only on your local machine)

One thought: the server returns status code 200 (OK) in this case, but if we start from this page (which doesn't exist on stable yet) then we get status code 404 instead. So actually I think we have access to the info that we need. Maybe in another PR I'll bang my head against this again.

@drammock
Copy link
Collaborator

when I run nox -s docs-live with this branch locally, I see the same CORS error as on the PR build, and I always get sent to the homepage (not the page I'm currently viewing). Are you actually seeing correct behaviour when viewing locally?

@12rambau
Copy link
Collaborator Author

12rambau commented Jun 20, 2023

Ok now I get why I manage to make it work, I customized my chrome instance to remove CORS (100% unsafe) for testing purposes.
We will be force to push to main to see an actual build and make a test without domain conflicts as all the other version live in "pydata-sphinx-theme.readthedocs.io". This is super upseting.

@drammock
Copy link
Collaborator

I customized my chrome instance to remove CORS (100% unsafe) for testing purposes

ha. that explains it. Well, let's take the plunge and see if it works on main 🤿

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.

Redirecting to homepage broken for version switcher
2 participants