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

Only put platforms list is less than 10, otherwise load with AJAX #2181

Merged
merged 14 commits into from
Sep 7, 2023

Conversation

GuillaumeGomez
Copy link
Member

Based on the same principle as #2113.

This PR does actually two things: only load releases if the "big menu" is opened and not if any menu is open. It allows to only load if needed even further.

The second part is actually adding the async load for the targets if there are more than 5 (having libc in mind for that).

If this approach looks good to you, I'll finish the PR by adding tests.

@github-actions github-actions bot added the S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed label Jul 31, 2023
@GuillaumeGomez
Copy link
Member Author

cc @Nemo157

Copy link
Member

@Nemo157 Nemo157 left a comment

Choose a reason for hiding this comment

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

I assume this is a significant part of the html sent for libc currently, have you measured how much it is?

src/web/routes.rs Outdated Show resolved Hide resolved
static/menu.js Outdated Show resolved Hide resolved
src/web/crate_details.rs Outdated Show resolved Hide resolved
templates/rustdoc/topbar.html Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

I assume this is a significant part of the html sent for libc currently, have you measured how much it is?

It's currently 18.972 bytes. https://docs.rs/libc/latest/libc/ is 888.749 bytes. So it roughly represents 2.13%. Not that much for big pages, but for pages like https://docs.rs/libc/latest/libc/constant.ABDAY_1.html (which is 55.753 bytes), it represents 34%.

@GuillaumeGomez GuillaumeGomez changed the title Only put platforms list is less than 6, otherwise load with AJAX Only put platforms list is less than 10, otherwise load with AJAX Aug 8, 2023
@GuillaumeGomez GuillaumeGomez marked this pull request as ready for review August 8, 2023 20:38
@GuillaumeGomez GuillaumeGomez force-pushed the ajax-platforms branch 3 times, most recently from bd79c9d to 221fef4 Compare August 8, 2023 21:11
@GuillaumeGomez
Copy link
Member Author

I added test for this feature and updated the failing ones (needed to update URLs mostly). It's now ready for review!

src/web/crate_details.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

Updated the routes.

src/web/routes.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

Fixed the target-redirect and updated the test checking that to ensure it's always set to the expected value. I also reverted the blacklist change I wrongly made.

src/web/crate_details.rs Outdated Show resolved Hide resolved
src/web/crate_details.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

Fixed the bug, improved the test a bit and updated the csp test.

src/web/crate_details.rs Outdated Show resolved Hide resolved
@Nemo157
Copy link
Member

Nemo157 commented Aug 26, 2023

Found a couple more bugs clicking around pages:

  • /crate/libc/0.2.147/source/ requests /-/menus/platforms/crate/libc/0.2.147/source/ which 404's
  • /crate/libc/0.2.147/builds/1436 gets a list, but they have links like /libc/0.2.147/arm-unknown-linux-musleabi/libc/1436 which search for 1436 in the docs
  • /libc/0.2.147/libc/struct.Elf32_Chdr.html works correctly, but /libc/0.2.147/aarch64-unknown-linux-gnu/libc/struct.Elf32_Chdr.html gets links to paths like /crate/libc/0.2.147/target-redirect/arm-unknown-linux-gnueabi/libc/libc/struct.Elf32_Chdr.html, the doubled /libc/libc/ internal path causes it to not find the target file and fallback to a search.

@GuillaumeGomez
Copy link
Member Author

Fixed all the URLs I could think of in /crate, which allowed me to discover cases where the URLs have a target-redirect whereas they shouldn't. You can see the cases in the tests I added.

@Nemo157
Copy link
Member

Nemo157 commented Aug 27, 2023

I think that LGTM now, can't find any more issues clicking around 🎉

It might be good to have @syphar or someone to confirm the CSP change is fine, I can't think of any issues it could cause but I'm not that familiar with it.

@GuillaumeGomez
Copy link
Member Author

🎉 Thanks for the review! I'll send a follow-up once merged about the target-redirect issue I mentioned.

Copy link
Member

@syphar syphar left a comment

Choose a reason for hiding this comment

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

reading about connect-src I believe the CSP change is fine.

Apart from that:
Looking back at #2113 we used the /:crate/ prefix so the cache invalidation would cover the release-list. While we don't return a caching header in get_all_releases (yet, which I definitely overlooked), we should keep CDN caching in mind also for the platform list fetching.

And using the /-/ prefix would bring the need to either add a new path to invalidations, or perhaps introduce a different prefix? ( I could imagine /:crate/-/..., if that doesn't overlap with rustdoc)

src/web/crate_details.rs Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Aug 30, 2023

Changed the menus URLs to start with /crate/:name/:version/menus.

@GuillaumeGomez
Copy link
Member Author

Anything else that remains to be done here?

@syphar
Copy link
Member

syphar commented Sep 6, 2023

Anything else that remains to be done here?

sorry for the delay, from my side I see that the routes were changed.
We could now also start actually caching the response, but I could imagine adding this separately later, especially since most platform lists will be directly rendered into the page.

I think a last manual test after the route change would be good, if @Nemo157 doesn't have time I could dig into this.

Copy link
Member

@Nemo157 Nemo157 left a comment

Choose a reason for hiding this comment

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

Just did another quick manual test and couldn't find any issues :shipit:

@GuillaumeGomez
Copy link
Member Author

🎉

@GuillaumeGomez GuillaumeGomez merged commit b2c47fa into rust-lang:master Sep 7, 2023
6 checks passed
@GuillaumeGomez GuillaumeGomez deleted the ajax-platforms branch September 7, 2023 09:32
@github-actions github-actions bot added S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it and removed S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed labels Sep 7, 2023
@syphar syphar removed the S-waiting-on-deploy This PR is ready to be merged, but is waiting for an admin to have time to deploy it label Sep 8, 2023
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.

3 participants