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.

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

app: Make separate requests for crate and versions #10296

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

app: Make separate requests for crate and versions #10296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
68adfee
crate: load `versions` with `crate.loadVersionsTask`
eth3lbert Dec 19, 2024
e3daaef
crate: Add reload support for `loadVersionsTask`
eth3lbert Dec 19, 2024
1a1b57b
crate: `findRecord` requests for `crate` without including `versions`…
eth3lbert Dec 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions app/adapters/crate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,15 @@ const BULK_REQUEST_GROUP_SIZE = 10;
export default class CrateAdapter extends ApplicationAdapter {
coalesceFindRequests = true;

findRecord(store, type, id, snapshot) {
let { include } = snapshot;
// This ensures `crate.versions` are always fetched from another request.
if (include === undefined) {
snapshot.include = 'keywords,categories,downloads';
}
return super.findRecord(store, type, id, snapshot);
}

groupRecordsForFindMany(store, snapshots) {
let result = [];
for (let i = 0; i < snapshots.length; i += BULK_REQUEST_GROUP_SIZE) {
Expand Down
6 changes: 4 additions & 2 deletions app/models/crate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,10 @@ export default class Crate extends Model {
return [...(teams ?? []), ...(users ?? [])];
});

loadVersionsTask = task(async () => {
return (await this.versions) ?? [];
loadVersionsTask = task(async ({ reload = false } = {}) => {
let versionsRef = this.hasMany('versions');
let fut = reload === true ? versionsRef.reload() : versionsRef.load();
return (await fut) ?? [];
});
}

Expand Down
2 changes: 1 addition & 1 deletion app/routes/crate/dependencies.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export default class VersionRoute extends Route {

async model() {
let crate = this.modelFor('crate');
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;
Comment on lines -9 to 12
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 •
edited
Loading

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 🤔

Expand Down
2 changes: 1 addition & 1 deletion app/routes/crate/range.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export default class VersionRoute extends Route {
let crate = this.modelFor('crate');

try {
let versions = await crate.hasMany('versions').load();
let versions = await crate.loadVersionsTask.perform();
let allVersionNums = versions.map(it => it.num);
let unyankedVersionNums = versions.filter(it => !it.yanked).map(it => it.num);

Expand Down
2 changes: 1 addition & 1 deletion app/routes/crate/version-dependencies.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ export default class VersionRoute extends Route {

let versions;
try {
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

} catch (error) {
let title = `${crate.name}: Failed to load version data`;
return this.router.replaceWith('catch-all', { transition, error, title, tryAgain: true });
Expand Down
2 changes: 1 addition & 1 deletion app/routes/crate/version.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export default class VersionRoute extends Route {

let versions;
try {
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

} catch (error) {
let title = `${crate.name}: Failed to load version data`;
return this.router.replaceWith('catch-all', { transition, error, title, tryAgain: true });
Expand Down
8 changes: 0 additions & 8 deletions e2e/acceptance/crate-dependencies.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,14 +75,6 @@ test.describe('Acceptance | crate dependencies page', { tag: '@acceptance' }, ()
server.get('/api/v1/crates/:crate_name/versions', {}, 500);
});

await ember.addHook(async owner => {
// Load `crate` and then explicitly unload the side-loaded `versions`.
let store = owner.lookup('service:store');
let crateRecord = await store.findRecord('crate', 'foo');
let versions = crateRecord.hasMany('versions').value();
versions.forEach(record => record.unloadRecord());
});

await page.goto('/crates/foo/1.0.0/dependencies');

await expect(page).toHaveURL('/crates/foo/1.0.0/dependencies');
Expand Down
8 changes: 0 additions & 8 deletions e2e/routes/crate/range.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,14 +127,6 @@ test.describe('Route | crate.range', { tag: '@routes' }, () => {
server.get('/api/v1/crates/:crate_name/versions', {}, 500);
});

await ember.addHook(async owner => {
// Load `crate` and then explicitly unload the side-loaded `versions`.
let store = owner.lookup('service:store');
let crateRecord = await store.findRecord('crate', 'foo');
let versions = crateRecord.hasMany('versions').value();
versions.forEach(record => record.unloadRecord());
});

await page.goto('/crates/foo/range/^3');
await expect(page).toHaveURL('/crates/foo/range/%5E3');
await expect(page.locator('[data-test-404-page]')).toBeVisible();
Expand Down
6 changes: 0 additions & 6 deletions tests/acceptance/crate-dependencies-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,12 +78,6 @@ module('Acceptance | crate dependencies page', function (hooks) {

this.server.get('/api/v1/crates/:crate_name/versions', {}, 500);

// Load `crate` and then explicitly unload the side-loaded `versions`.
let store = this.owner.lookup('service:store');
let crateRecord = await store.findRecord('crate', 'foo');
let versions = crateRecord.hasMany('versions').value();
versions.forEach(record => record.unloadRecord());

await visit('/crates/foo/1.0.0/dependencies');
assert.strictEqual(currentURL(), '/crates/foo/1.0.0/dependencies');
assert.dom('[data-test-404-page]').exists();
Expand Down
17 changes: 17 additions & 0 deletions tests/adapters/crate-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,21 @@ module('Adapter | crate', function (hooks) {
assert.strictEqual(foo?.name, 'foo');
assert.strictEqual(bar?.name, 'bar');
});

test('findRecord requests do not include versions by default', async function (assert) {
let _foo = this.server.create('crate', { name: 'foo' });
let version = this.server.create('version', { crate: _foo });

let store = this.owner.lookup('service:store');

let foo = await store.findRecord('crate', 'foo');
assert.strictEqual(foo?.name, 'foo');

// versions should not be loaded yet
let versionsRef = foo.hasMany('versions');
assert.deepEqual(versionsRef.ids(), []);

await versionsRef.load();
assert.deepEqual(versionsRef.ids(), [version.id]);
});
});
2 changes: 1 addition & 1 deletion tests/models/version-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ module('Model | Version', function (hooks) {
this.server.create('version', { crate, num: '0.4.2' });
this.server.create('version', { crate, num: '0.4.3', yanked: true });
crateRecord = await this.store.findRecord('crate', crate.name, { reload: true });
versions = (await crateRecord.loadVersionsTask.perform()).slice();
versions = (await crateRecord.loadVersionsTask.perform({ reload: true })).slice();

assert.deepEqual(
versions.map(it => ({ num: it.num, isHighestOfReleaseTrack: it.isHighestOfReleaseTrack })),
Expand Down
6 changes: 0 additions & 6 deletions tests/routes/crate/range-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,12 +119,6 @@ module('Route | crate.range', function (hooks) {

this.server.get('/api/v1/crates/:crate_name/versions', {}, 500);

// Load `crate` and then explicitly unload the side-loaded `versions`.
let store = this.owner.lookup('service:store');
let crateRecord = await store.findRecord('crate', 'foo');
let versions = crateRecord.hasMany('versions').value();
versions.forEach(record => record.unloadRecord());

await visit('/crates/foo/range/^3');
assert.strictEqual(currentURL(), '/crates/foo/range/%5E3');
assert.dom('[data-test-404-page]').exists();
Expand Down
Loading