WIP: add all_versions_yanked #1141
Conversation
| let versions_link = match versions { | ||
| Some(..) => None, | ||
| None => Some(format!("/api/v1/crates/{}/versions", name)), | ||
| }; |
There was a problem hiding this comment.
Was this condition really necessary? (It removes the link if any versions are passed to encodable)
There was a problem hiding this comment.
I think this was from before we had encodable and minimal_encodable.
I'm seeing some more ajax requests but i'm not sure if it's this change or some other change in this PR causing it, will detail in an overall comment.
There was a problem hiding this comment.
Hi Sean!! First of all, I want to apologize for not being responsive at all to the questions you've had along the way :( You've done great despite my not-helpfulness!!
To answer your questions in the PR description:
- only show is applying an order to versions
- I would move ordering down into encodable, but show ends up using the variable again
show uses the versions variable again to return encoded version JSON, right? It looks like your current iteration of the code is cloning versions before passing it to Crate encodable, so that should allow for re-use, right? Or is there some other problem with that approach?
- minimal_encodable now produces an array of version numbers, when it didn't before
- unless we care about keeping the JSON response as small as possible, this doesn't seem to be a real issue
I agree, I'm not super concerned with JSON response size at this point.
One possible bug I noticed in the previous code was that the show action doesn't filter out yanked versions, while the index action does. Was that intentional, or an oversight?
So currently, on a crate show page, we do want to show yanked versions with (yanked) text next to yanked version numbers. Example: https://crates.io/crates/path-router shows in the sidebar that version 0.1.0 was yanked. So the show action indeed should not filter out yanked versions.
For the crate index route, I think the only thing versions are used for are figuring out the latest non-yanked, non-prerelease version number as the "current version" of the crate. So that seems correct and intentional to me as well.
Was there some behavior that seemed wrong due to this filtering, or was it just an inconsistency that caught your eye?
While testing this branch out, I'm seeing more ajax requests happen than in production. I think this is due to how and when Ember follows links to get associated information... but I'm not sure which code change is causing this behavior change. Here's what I'm seeing:
- In production, visit crates.io and click on libc. I see the following ajax requests that happen as a result of clicking on libc:
- https://crates.io/api/v1/crates/libc/versions
- https://crates.io/api/v1/crates/libc
- https://crates.io/api/v1/crates/libc/0.2.33/readme
- https://crates.io/api/v1/crates/libc/reverse_dependencies
- https://crates.io/api/v1/crates/libc/0.2.33/authors
- https://crates.io/api/v1/crates/libc/owner_team
- https://crates.io/api/v1/crates/libc/owner_user
- https://crates.io/api/v1/crates/libc/0.2.33/dependencies
- https://crates.io/api/v1/crates/libc/downloads
- When running crates.io locally and I visit localhost:4200 then click on a crate that has 6 versions, I see equivalent ajax requests that I see on production PLUS:
- http://localhost:4200/api/v1/versions/42
- http://localhost:4200/api/v1/versions/43
- etc for all 6 versions (that is, more than the 5 shown on the crate page)
I don't think anything on the crate show page needs this information, and I'm not sure why Ember is requesting these, but it might have something to do with why the links were the way they were? Do you know much Ember and does what I'm seeing make sense to you?
| let versions_link = match versions { | ||
| Some(..) => None, | ||
| None => Some(format!("/api/v1/crates/{}/versions", name)), | ||
| }; |
There was a problem hiding this comment.
I think this was from before we had encodable and minimal_encodable.
I'm seeing some more ajax requests but i'm not sure if it's this change or some other change in this PR causing it, will detail in an overall comment.
|
No worries; I figured you were busy with Rust Belt Rust.
The code works as it's currently written; I was saying that I would've wanted to make it so that
I just didn't understand the application's reasoning for the difference. It seems to make sense now.
The association is How do you usually seed your development database? Honestly I'm surprised that y'all are using Ember (or any heavy JS framework). At work we rewrote a Rails site w/ Ember, only to rewrite again with minimal JS because the site had grown too slow and the code duplication & obtuse bugs significantly slowed down development. |
|
I want to apologize for the delay in answering your questions, providing feedback, and generally moving this PR forward-- I haven't been a great maintainer lately. Since this PR was opened, we have done quite a bit of refactoring that I think has improved the codebase, but unfortunately has made updating PRs a bit tricky. I feel bad about this too, and normally I would take on the work of resolving merge conflicts. However, in order to try to get out from under the backlog of PRs that are outstanding, I've decided to aggressively close outdated PRs rather than update them. At this point it would probably take less time to reimplement the work rather than fix merge conflicts. I appreciate the work you've put in here, and again I sincerely apologize for my lack of response. If you would like to work on this again in the future, I would be honored, but I also totally understand if you'd rather not. Thank you! |
For #1058
I chose this approach in order to reduce duplication and prevent unnecessary database requests. For example,
max_versionwas calling the database even though the versions had already been eagerly loaded, and it duplicated the logic already inVersion::max.But there are plenty of issues with this approach. Some of them are:
showis applying an order toversionsencodable, butshowends up using the variable againminimal_encodablenow produces an array of version numbers, when it didn't beforeIs this sort of refactor welcome, and if so how do you think it could be improved?
One possible bug I noticed in the previous code was that the show action doesn't filter out yanked versions, while the index action does. Was that intentional, or an oversight?