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

app: Make separate requests for crate and versions #10296

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

eth3lbert
Copy link
Contributor

Extracted from #10248.

This will ensure that the versions are fetched via a separate requests, rather than being included in the crate's response. This not only enables us to render the page layout first for a faster first paint but also allows us to migrate to a paginated version list in the future.

Before:

full-versions-included.mov

After:

versions-not-included.mov

app/adapters/crate.js Outdated Show resolved Hide resolved
Comment on lines -9 to 12
let versions = await crate.get('versions');
let versions = await crate.loadVersionsTask.perform();

let { default_version } = crate;
let version = versions.find(version => version.num === default_version) ?? versions.lastObject;
Copy link
Member

Choose a reason for hiding this comment

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

doesn't have to be in this PR, but this should probably just this.router.replaceWith('crate.version-dependencies', crate, default_version); instead of loading the full version list 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the part after #10288 needs to be changed. I'm also wondering if we could just load versions in the versions route. (This would have the downside of not having a versions count on the tab.)

Copy link
Member

@Turbo87 Turbo87 Dec 31, 2024

Choose a reason for hiding this comment

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

I'm also wondering if we could just load versions in the versions route. (This would have the downside of not having a versions count on the tab.)

I forgot that we show the number of versions in the tab. I do look at that quite often though, so it would be unfortunate to lose it 🤔

versions = await crate.get('versions');
versions = await crate.loadVersionsTask.perform();
Copy link
Member

Choose a reason for hiding this comment

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

again, not in this PR: but this should probably also only request the corresponding version instead of the full version list :D

versions = await crate.get('versions');
versions = await crate.loadVersionsTask.perform();
Copy link
Member

Choose a reason for hiding this comment

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

same as above, this shouldn't need to load the full version list to display a single version

e2e/acceptance/crate-dependencies.spec.ts Outdated Show resolved Hide resolved
@eth3lbert eth3lbert force-pushed the app-crate-without-versions branch 2 times, most recently from bdf7ddd to 698a82c Compare December 31, 2024 10:41
… by default

This will ensure that the `versions` are fetched via a separate
requests, rather than being included in the crate's response. This not
only enables us to render the page layout first for a faster first paint
but also allows us to migrate to a paginated version list in the future.
@eth3lbert eth3lbert force-pushed the app-crate-without-versions branch from 698a82c to 1a1b57b Compare January 4, 2025 03:29
@eth3lbert
Copy link
Contributor Author

rebased!

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

Successfully merging this pull request may close these issues.

2 participants