From b9ba7c13570019a4d4c07250eba3492cbf9b06ee Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Wed, 8 Jan 2025 23:30:02 +0800 Subject: [PATCH 1/5] app: Include `default_version` by default 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. --- app/adapters/crate.js | 2 +- tests/adapters/crate-test.js | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/app/adapters/crate.js b/app/adapters/crate.js index 18b4badf87b..bac2eaac894 100644 --- a/app/adapters/crate.js +++ b/app/adapters/crate.js @@ -49,7 +49,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; diff --git a/tests/adapters/crate-test.js b/tests/adapters/crate-test.js index 86e84e97d13..2ad6571b231 100644 --- a/tests/adapters/crate-test.js +++ b/tests/adapters/crate-test.js @@ -29,18 +29,26 @@ module('Adapter | crate', function (hooks) { test('findRecord requests do not include versions by default', async function (assert) { let _foo = this.db.crate.create({ name: 'foo' }); - let version = this.db.version.create({ crate: _foo }); + this.db.version.create({ crate: _foo, num: '0.0.1' }); + this.db.version.create({ crate: _foo, num: '0.0.2' }); + this.db.version.create({ crate: _foo, num: '0.0.3' }); + + let versions = this.db.version.getAll().reverse(); + let default_version = versions.find(it => it.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(it => `${it.id}`), + ); }); }); From 3b34e6753f10d48ec7f74faa99ccb0d550fad99c Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Wed, 8 Jan 2025 12:25:32 +0800 Subject: [PATCH 2/5] routes/crate/dependencies: Redirect directly to `default_version` --- app/routes/crate/dependencies.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/app/routes/crate/dependencies.js b/app/routes/crate/dependencies.js index 8ffa3c3aa8a..68334f99636 100644 --- a/app/routes/crate/dependencies.js +++ b/app/routes/crate/dependencies.js @@ -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); } } From c5da70b9863a277a79d3929901b1f714042c4c78 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Wed, 8 Jan 2025 12:42:00 +0800 Subject: [PATCH 3/5] adapters/version: Add `queryRecord` support for `GET /api/v1/crate/{name}/{version}` endpoint --- app/adapters/version.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/app/adapters/version.js b/app/adapters/version.js index c8be99bdd27..bf2c4739868 100644 --- a/app/adapters/version.js +++ b/app/adapters/version.js @@ -6,4 +6,14 @@ export default class VersionAdapter extends ApplicationAdapter { let num = snapshot.record.num; return `/${this.namespace}/crates/${crateName}/${num}`; } + + urlForQueryRecord(query) { + let { name, num } = query ?? {}; + let baseUrl = this.buildURL('crate', name); + let url = `${baseUrl}/${num}`; + // The following used to remove them from URL's query string. + delete query.name; + delete query.num; + return url; + } } From 529f8a359ad5bcf14d9e36d822e6aa37031cf548 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Wed, 8 Jan 2025 12:44:50 +0800 Subject: [PATCH 4/5] models/crate: Add `loadedVersionsByNum` for lookup `versions` by `num` --- app/models/crate.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/models/crate.js b/app/models/crate.js index cc73c860eb9..67b520a0c9e 100644 --- a/app/models/crate.js +++ b/app/models/crate.js @@ -67,6 +67,14 @@ export default class Crate extends Model { return Object.fromEntries(versions.slice().map(v => [v.id, v])); } + /** @return {Map} */ + @cached + get loadedVersionsByNum() { + let versionsRef = this.hasMany('versions'); + let values = versionsRef.value(); + return new Map(values?.map(ref => [ref.num, ref])); + } + @cached get releaseTrackSet() { let map = new Map(); let { versionsObj: versions, versionIdsBySemver } = this; From 4d95228d0cc1ce839593690580dd3f73bb359f47 Mon Sep 17 00:00:00 2001 From: eth3lbert Date: Wed, 8 Jan 2025 20:23:18 +0800 Subject: [PATCH 5/5] routes/crate: Get or fetch requested version by `num` 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. --- app/routes/crate/version-dependencies.js | 28 ++++++++------ app/routes/crate/version.js | 42 ++++++++++++--------- e2e/acceptance/crate-dependencies.spec.ts | 14 ------- tests/acceptance/crate-dependencies-test.js | 14 ------- 4 files changed, 42 insertions(+), 56 deletions(-) diff --git a/app/routes/crate/version-dependencies.js b/app/routes/crate/version-dependencies.js index 75068fd7af4..9a85d43cafa 100644 --- a/app/routes/crate/version-dependencies.js +++ b/app/routes/crate/version-dependencies.js @@ -1,25 +1,31 @@ +import { NotFoundError } from '@ember-data/adapter/error'; 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; + let requestedVersion = params.version_num; + let version; try { - versions = await crate.loadVersionsTask.perform(); + version = + crate.loadedVersionsByNum.get(requestedVersion) ?? + (await this.store.queryRecord('version', { + name: crate.id, + num: requestedVersion, + })); } 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); - if (!version) { - let title = `${crate.name}: Version ${requestedVersion} not found`; - return this.router.replaceWith('catch-all', { transition, title }); + if (error instanceof NotFoundError) { + let title = `${crate.name}: Version ${requestedVersion} not found`; + return this.router.replaceWith('catch-all', { transition, title }); + } else { + let title = `${crate.name}: Failed to load version data`; + return this.router.replaceWith('catch-all', { transition, error, title, tryAgain: true }); + } } try { diff --git a/app/routes/crate/version.js b/app/routes/crate/version.js index 683fbf8f3fd..a18450c7039 100644 --- a/app/routes/crate/version.js +++ b/app/routes/crate/version.js @@ -1,22 +1,26 @@ +import { NotFoundError } from '@ember-data/adapter/error'; import Route from '@ember/routing/route'; import { inject as service } from '@ember/service'; import { waitForPromise } from '@ember/test-waiters'; import { didCancel } from 'ember-concurrency'; -import semverSort from 'semver/functions/rsort'; 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'); - let versions; + // TODO: Resolved version without waiting for versions to be resolved + // The main blocker for this right now is that we have a "belongsTo" relationship between + // `version-download` and `version` in sync mode. This requires us to wait for `versions` to + // exist before `version-download` can be created. try { - versions = await crate.loadVersionsTask.perform(); + 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 }); @@ -24,21 +28,25 @@ export default class VersionRoute extends Route { let version; let requestedVersion = params.version_num; - if (requestedVersion) { - version = versions.find(version => version.num === requestedVersion); - 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); - - if (!version) { - let versionNums = versions.map(it => it.num); - semverSort(versionNums, { loose: true }); + let num = requestedVersion || crate.default_version; - version = versions.find(version => version.num === versionNums[0]); + try { + version = + crate.loadedVersionsByNum.get(num) ?? + (await crate.store.queryRecord('version', { + name: crate.id, + num, + })); + } catch (error) { + if (error instanceof NotFoundError) { + let title = + requestedVersion == null + ? `${crate.name}: Failed to find default version` + : `${crate.name}: Version ${requestedVersion} not found`; + return this.router.replaceWith('catch-all', { transition, title }); + } else { + let title = `${crate.name}: Failed to load version data`; + return this.router.replaceWith('catch-all', { transition, error, title, tryAgain: true }); } } diff --git a/e2e/acceptance/crate-dependencies.spec.ts b/e2e/acceptance/crate-dependencies.spec.ts index ac792096dd0..401c2cb1673 100644 --- a/e2e/acceptance/crate-dependencies.spec.ts +++ b/e2e/acceptance/crate-dependencies.spec.ts @@ -62,20 +62,6 @@ test.describe('Acceptance | crate dependencies page', { tag: '@acceptance' }, () await expect(page.locator('[data-test-try-again]')).toHaveCount(0); }); - test('shows an error page if versions fail to load', async ({ page, msw, ember }) => { - let crate = msw.db.crate.create({ name: 'foo' }); - msw.db.version.create({ crate, num: '2.0.0' }); - await msw.worker.use(http.get('/api/v1/crates/:crate_name/versions', () => HttpResponse.json({}, { status: 500 }))); - - await page.goto('/crates/foo/1.0.0/dependencies'); - - await expect(page).toHaveURL('/crates/foo/1.0.0/dependencies'); - await expect(page.locator('[data-test-404-page]')).toBeVisible(); - await expect(page.locator('[data-test-title]')).toHaveText('foo: Failed to load version data'); - await expect(page.locator('[data-test-go-back]')).toHaveCount(0); - await expect(page.locator('[data-test-try-again]')).toBeVisible(); - }); - test('shows error message if loading of dependencies fails', async ({ page, msw }) => { let crate = msw.db.crate.create({ name: 'foo' }); msw.db.version.create({ crate, num: '1.0.0' }); diff --git a/tests/acceptance/crate-dependencies-test.js b/tests/acceptance/crate-dependencies-test.js index 29fb4ef6be9..a34139ab132 100644 --- a/tests/acceptance/crate-dependencies-test.js +++ b/tests/acceptance/crate-dependencies-test.js @@ -74,20 +74,6 @@ module('Acceptance | crate dependencies page', function (hooks) { assert.dom('[data-test-try-again]').doesNotExist(); }); - test('shows an error page if versions fail to load', async function (assert) { - let crate = this.db.crate.create({ name: 'foo' }); - this.db.version.create({ crate, num: '2.0.0' }); - - this.worker.use(http.get('/api/v1/crates/:crate_name/versions', () => HttpResponse.json({}, { status: 500 }))); - - await visit('/crates/foo/1.0.0/dependencies'); - assert.strictEqual(currentURL(), '/crates/foo/1.0.0/dependencies'); - assert.dom('[data-test-404-page]').exists(); - assert.dom('[data-test-title]').hasText('foo: Failed to load version data'); - assert.dom('[data-test-go-back]').doesNotExist(); - assert.dom('[data-test-try-again]').exists(); - }); - test('shows error message if loading of dependencies fails', async function (assert) { let crate = this.db.crate.create({ name: 'foo' }); this.db.version.create({ crate, num: '1.0.0' });