Merge pull request #10344 from eth3lbert/app-default-version

Include `default_version` by default

Author: Turbo87

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;

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;
+  }
 }

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<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]));
+  }
+
   @cached get releaseTrackSet() {
     let map = new Map();
     let { versionsObj: versions, versionIdsBySemver } = this;

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);
   }
 }

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 {

app/routes/crate/version.js

@@ -1,44 +1,52 @@
+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 });
     }
 
     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 });
       }
     }
 

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' });

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' });

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}`),
+    );
   });
 });