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

Add beta version of doc diff library for testing #9546

Merged
merged 2 commits into from
Mar 7, 2023
Merged

Conversation

agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Aug 24, 2022

Very slim chance this works as expected


📚 Documentation previews 📚

@agjohnson agjohnson requested a review from a team as a code owner August 24, 2022 16:36
@agjohnson
Copy link
Contributor Author

Yeah, CORS error, about what I expected.

Cross-Origin Request Blocked: The Same Origin Policy disallows reading the remote resource at https://docs.readthedocs.io/en/latest/versions.html. (Reason: CORS header ‘Access-Control-Allow-Origin’ missing).

Page that should have a diff: https://docs--9546.org.readthedocs.build/en/9546/versions.html

@humitos
Copy link
Member

humitos commented Aug 24, 2022

This works pretty nice! (I had to disable the CORS restriction in my browser, tho)

Screenshot_2022-08-24_19-18-48


I noticed that it detected a weird change in the table, tho. It seems that table was not changed in the PR.

Screenshot_2022-08-24_19-19-20

@agjohnson
Copy link
Contributor Author

I noticed that it detected a weird change in the table, tho

That is odd, the HTML is identical 🤔

@agjohnson
Copy link
Contributor Author

I opened readthedocs/addons#93 to discuss CORS and authentication mitigation at a broad level.

@ericholscher
Copy link
Member

This is wonderful! Excited to figure out the CORS issue, and have some of this magic in our PR's 🎉

@humitos
Copy link
Member

humitos commented Oct 6, 2022

@stsewd how hard would be to enable CORS for this particular case? I'd love to see this PR merged and in action for our repository at least.

@stsewd
Copy link
Member

stsewd commented Oct 6, 2022

@humitos there is an issue related to this at https://github.com/readthedocs/readthedocs-corporate/issues/1482. We should be safe to allow cross site requests on .org, since everything is public, and we don't use cookies on the docs domains (we could even restrict this for domains that are valid on .org only).

@ericholscher
Copy link
Member

ericholscher commented Nov 23, 2022

We could use a CORS proxy for this. I already have one running in workers for WTD we could test against: https://cors.writethedocs.workers.dev/corsproxy/?apiurl=https://docs.readthedocs.io/en/stable/

@agjohnson
Copy link
Contributor Author

image

@ericholscher
Copy link
Member

Fancy :)

@humitos
Copy link
Member

humitos commented Nov 24, 2022

@agjohnson
Copy link
Contributor Author

Note, what we added was a hack. I'd wait to merge this until we have a proxy maintained on the RTD side.

@humitos
Copy link
Member

humitos commented Mar 6, 2023

I know that we have worked around on some CORS issues already. Are we ready to merge this PR and start using internally the doc-diff javascript library?

@ericholscher
Copy link
Member

I'm 👍 on it! Dogfooding ftw.

@humitos
Copy link
Member

humitos commented Mar 7, 2023

OK, this is looking good so far:

Screenshot_2023-03-07_10-43-30

I'm going to merge it and we can continue from there.

@humitos humitos enabled auto-merge (squash) March 7, 2023 09:44
@humitos humitos merged commit 0ba92d3 into main Mar 7, 2023
@humitos humitos deleted the agj/add-beta-doc-diff branch March 7, 2023 09:55
@ericholscher
Copy link
Member

Nice.

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

Successfully merging this pull request may close these issues.

4 participants