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

Include default_version by default #10344

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion app/adapters/crate.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export default class CrateAdapter extends ApplicationAdapter {
function setDefaultInclude(query) {
if (query.include === undefined) {
// This ensures `crate.versions` are always fetched from another request.
query.include = 'keywords,categories,downloads';
query.include = 'keywords,categories,downloads,default_version';
}

return query;
Expand Down
8 changes: 8 additions & 0 deletions app/adapters/version.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,12 @@ export default class VersionAdapter extends ApplicationAdapter {
let num = snapshot.record.num;
return `/${this.namespace}/crates/${crateName}/${num}`;
}

queryRecord(_store, _type, query) {
let { name, num } = query ?? {};
let baseUrl = this.buildURL('crate', name);
let url = `${baseUrl}/${num}`;
let data = { ...query, name: undefined, num: undefined };
return this.ajax(url, 'GET', { data });
}
eth3lbert marked this conversation as resolved.
Show resolved Hide resolved
}
8 changes: 8 additions & 0 deletions app/models/crate.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ export default class Crate extends Model {
return Object.fromEntries(versions.slice().map(v => [v.id, v]));
}

/** @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]));
}
Comment on lines +70 to +76
Copy link
Contributor Author

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.


@cached get releaseTrackSet() {
let map = new Map();
let { versionsObj: versions, versionIdsBySemver } = this;
Expand Down
5 changes: 1 addition & 4 deletions app/routes/crate/dependencies.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,8 @@ export default class VersionRoute extends Route {

async model() {
let crate = this.modelFor('crate');
let versions = await crate.loadVersionsTask.perform();

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

this.router.replaceWith('crate.version-dependencies', crate, version.num);
this.router.replaceWith('crate.version-dependencies', crate, default_version);
}
}
31 changes: 22 additions & 9 deletions app/routes/crate/version-dependencies.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,23 @@ import Route from '@ember/routing/route';
import { inject as service } from '@ember/service';

export default class VersionRoute extends Route {
@service store;
@service router;

async model(params, transition) {
let crate = this.modelFor('crate');

let versions;
try {
versions = await crate.loadVersionsTask.perform();
} catch (error) {
let title = `${crate.name}: Failed to load version data`;
return this.router.replaceWith('catch-all', { transition, error, title, tryAgain: true });
}

let requestedVersion = params.version_num;
let version = versions.find(version => version.num === requestedVersion);
let version =
crate.loadedVersionsByNum.get(requestedVersion) ??
(await this.store
.queryRecord('version', {
name: crate.id,
num: requestedVersion,
})
.catch(() => {
// ignored
}));
if (!version) {
let title = `${crate.name}: Version ${requestedVersion} not found`;
return this.router.replaceWith('catch-all', { transition, title });
Expand All @@ -32,6 +34,17 @@ export default class VersionRoute extends Route {
return version;
}

async afterModel(_resolvedModel, transition) {
let crate = this.modelFor('crate');
// TODO: Resolved version without waiting for versions to be resolved
try {
await crate.loadVersionsTask.perform();
} catch (error) {
let title = `${crate.name}: Failed to load version data`;
return this.router.replaceWith('catch-all', { transition, error, title, tryAgain: true });
}
}

setupController(controller, model) {
controller.set('version', model);
controller.set('crate', this.modelFor('crate'));
Expand Down
25 changes: 23 additions & 2 deletions app/routes/crate/version.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@ import { AjaxError } from '../../utils/ajax';
export default class VersionRoute extends Route {
@service router;
@service sentry;
@service store;

async model(params, transition) {
let crate = this.modelFor('crate');

// TODO: Resolved version without waiting for versions to be resolved
let versions;
try {
versions = await crate.loadVersionsTask.perform();
Expand All @@ -25,14 +27,33 @@ export default class VersionRoute extends Route {
let version;
let requestedVersion = params.version_num;
if (requestedVersion) {
version = versions.find(version => version.num === requestedVersion);
version =
crate.loadedVersionsByNum.get(requestedVersion) ??
(await this.store
.queryRecord('version', {
name: crate.id,
num: requestedVersion,
})
.catch(() => {
// ignored
}));

if (!version) {
let title = `${crate.name}: Version ${requestedVersion} not found`;
return this.router.replaceWith('catch-all', { transition, title });
}
} else {
let { default_version } = crate;
version = versions.find(version => version.num === default_version);
version =
crate.loadedVersionsByNum.get(default_version) ??
(await this.store
.queryRecord('version', {
name: crate.id,
num: default_version,
})
.catch(() => {
// ignored
}));

if (!version) {
let versionNums = versions.map(it => it.num);
Expand Down
2 changes: 1 addition & 1 deletion e2e/acceptance/crate-dependencies.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ test.describe('Acceptance | crate dependencies page', { tag: '@acceptance' }, ()
test('shows an error page if versions fail to load', async ({ page, mirage, ember }) => {
await mirage.addHook(server => {
let crate = server.create('crate', { name: 'foo' });
server.create('version', { crate, num: '2.0.0' });
server.create('version', { crate, num: '1.0.0' });
server.get('/api/v1/crates/:crate_name/versions', {}, 500);
});

Expand Down
2 changes: 1 addition & 1 deletion tests/acceptance/crate-dependencies-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ module('Acceptance | crate dependencies page', function (hooks) {

test('shows an error page if versions fail to load', async function (assert) {
let crate = this.server.create('crate', { name: 'foo' });
this.server.create('version', { crate, num: '2.0.0' });
this.server.create('version', { crate, num: '1.0.0' });

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

Expand Down
20 changes: 14 additions & 6 deletions tests/adapters/crate-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,20 +23,28 @@ module('Adapter | crate', function (hooks) {
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 });
test('findRecord requests only include `default_version` by default', async function (assert) {
let crate = this.server.create('crate', { name: 'foo' });
let versions = [
this.server.create('version', { crate, num: '0.0.1' }),
this.server.create('version', { crate, num: '0.0.2' }),
this.server.create('version', { crate, num: '0.0.3' }),
];
let default_version = versions.find(v => v.num === '0.0.3');

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
// Only `defaul_version` should be loaded
let versionsRef = foo.hasMany('versions');
assert.deepEqual(versionsRef.ids(), []);
assert.deepEqual(versionsRef.ids(), [default_version.id]);

await versionsRef.load();
assert.deepEqual(versionsRef.ids(), [version.id]);
assert.deepEqual(
versionsRef.ids(),
versions.map(v => v.id),
);
});
});
Loading