Skip to content

Include default_version by default #10344

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Feb 6, 2025
Merged

Include default_version by default #10344

merged 5 commits into from
Feb 6, 2025

Conversation

@eth3lbert
Copy link
Contributor

@eth3lbert eth3lbert commented Jan 8, 2025

This PR would ensure that default_version is included in the crate response by default. This eliminates the need to wait for the versions request to complete when default_version is the only version required. This also decouples requested version handling from versions.

Related:

@eth3lbert eth3lbert force-pushed the app-default-version branch from a03882a to ddfede2 Compare January 8, 2025 16:25
eth3lbert
eth3lbert commented Jan 8, 2025
View reviewed changes
app/models/crate.js
Comment on lines +70 to +76
/** @return {Map<string, import("../models/version").default>} */
@cached
get loadedVersionsByNum() {
let versionsRef = this.hasMany('versions');
let values = versionsRef.value();
return new Map(values?.map(ref => [ref.num, ref]));
}
Copy link
Contributor Author

@eth3lbert eth3lbert Jan 8, 2025

Choose a reason for hiding this comment

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

I think versionsObj could also be changed to implement in this way. This would allow all versions that belong to this crate to be available, rather than just those loaded via the loadVersionsTask.

@eth3lbert
Copy link
Contributor Author

eth3lbert commented Jan 8, 2025

I have removed a commit that aimed to only await versions when needed. This commit fails in CI because we have some versions.length in the template, which would potentially load versions. I will address this in a separate PR, which will also require that the total number of versions be extracted from the meta.

Turbo87
Turbo87 reviewed Jan 9, 2025
View reviewed changes
@eth3lbert eth3lbert force-pushed the app-default-version branch from ddfede2 to bfe8d72 Compare January 10, 2025 07:31
@eth3lbert
Copy link
Contributor Author

eth3lbert commented Jan 10, 2025

Rebased to make CI happy after the new Danger Zone was added :D

@eth3lbert eth3lbert force-pushed the app-default-version branch 2 times, most recently from 96786e2 to 433abe7 Compare January 13, 2025 14:09
@eth3lbert eth3lbert force-pushed the app-default-version branch from 433abe7 to 59a975b Compare January 22, 2025 14:16
@eth3lbert
Copy link
Contributor Author

eth3lbert commented Jan 22, 2025

Rebased!

Turbo87
Turbo87 reviewed Jan 23, 2025
View reviewed changes
@Turbo87
Copy link
Member

Turbo87 commented Jan 23, 2025

also, sorry for keeping you waiting on this. I totally forgot about this PR 😞

@eth3lbert eth3lbert force-pushed the app-default-version branch 2 times, most recently from 901fc09 to 859959f Compare January 23, 2025 15:07
@eth3lbert eth3lbert requested a review from Turbo87 January 23, 2025 15:09
eth3lbert
eth3lbert commented Jan 25, 2025
View reviewed changes
@eth3lbert
app: Include default_version by default
b9ba7c1 
This commit would ensure that `default_version` is included in the
`crate` response by default. This eliminates the need to wait for the
`versions` request to complete when `default_version` is the only
version required.
@eth3lbert
routes/crate/dependencies: Redirect directly to default_version
3b34e67
@eth3lbert
adapters/version: Add queryRecord support for `GET /api/v1/crate/{n…
c5da70b 
…ame}/{version}` endpoint
@eth3lbert
models/crate: Add loadedVersionsByNum for lookup versions by num
529f8a3
@eth3lbert eth3lbert force-pushed the app-default-version branch from 859959f to 37d2806 Compare February 3, 2025 09:16
@eth3lbert
Copy link
Contributor Author

eth3lbert commented Feb 3, 2025

Conflict resolved!

@Turbo87 Just a friendly ping in case you forgot about this PR. 😉

Turbo87
Turbo87 reviewed Feb 4, 2025
View reviewed changes
@eth3lbert eth3lbert force-pushed the app-default-version branch 3 times, most recently from 666588d to 1644694 Compare February 4, 2025 14:28
Turbo87
Turbo87 reviewed Feb 5, 2025
View reviewed changes
@eth3lbert
routes/crate: Get or fetch requested version by num
4d95228 
Previously, we fetched all versions and then found the requested version
within them. This commit changes the approach to either retrieve the
version from loaded versions or fetch it from the endpoint. This change
allows us to migrate to paginated versions in the future.
@eth3lbert eth3lbert force-pushed the app-default-version branch from 1644694 to 4d95228 Compare February 5, 2025 10:29
Turbo87
Turbo87 approved these changes Feb 6, 2025
View reviewed changes
Copy link
Member

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

LGTM now :)

I guess this is good to go?

@eth3lbert
Copy link
Contributor Author

eth3lbert commented Feb 6, 2025

LGTM now :)

I guess this is good to go?

Yes, I will file a follow-up PR tomorrow :)

thanks for the review!

@Turbo87 Turbo87 merged commit de97fbd into rust-lang:main Feb 6, 2025
10 checks passed
@eth3lbert eth3lbert deleted the app-default-version branch February 7, 2025 16:17
@eth3lbert eth3lbert mentioned this pull request Feb 7, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Turbo87 Turbo87 Turbo87 approved these changes

Assignees

No one assigned

Labels

A-frontend 🐹

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@eth3lbert @Turbo87