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

Fix frontend deprecations #10198

Merged
merged 8 commits into from
Dec 15, 2024
Merged

Fix frontend deprecations #10198

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
bb5fc36
crate-sidebar: Fix deprecations
eth3lbert Dec 12, 2024
74c2d22
crate.index: Load `downloadsContext.version_downloads` explicitly wit…
eth3lbert Dec 12, 2024
2e536ff
models/crate: Make `owners` getter rely on `loadOwnersTask`
eth3lbert Dec 13, 2024
b09f410
models/crate: Add `hasOwnerUser()` fn that relies on `loadOwnerUserTask`
eth3lbert Dec 13, 2024
5cc896d
app: Simplify `isOwner` with `crate.hasOwnerUser(userId)` fn
eth3lbert Dec 13, 2024
42ca1c7
crate.settings: Remove owner by native array method `splice`
eth3lbert Dec 13, 2024
9538b22
models/crate: Load `crate.versions` explicitly with a task
eth3lbert Dec 13, 2024
ac1a774
tests/components/crate-sidebar: Use the native array method `[0]`
eth3lbert Dec 13, 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
28 changes: 22 additions & 6 deletions app/models/crate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,22 +37,34 @@ export default class Crate extends Model {
@hasMany('dependency', { async: true, inverse: null }) reverse_dependencies;

@cached get versionIdsBySemver() {
let versions = this.versions.toArray() ?? [];
return versions.sort(compareVersionBySemver).map(v => v.id);
let { last } = this.loadVersionsTask;
assert('`loadVersionsTask.perform()` must be called before calling `versionIdsBySemver`', last != null);
let versions = last?.value ?? [];
return versions
.slice()
.sort(compareVersionBySemver)
.map(v => v.id);
}

@cached get versionIdsByDate() {
let versions = this.versions.toArray() ?? [];
return versions.sort(compareVersionByDate).map(v => v.id);
let { last } = this.loadVersionsTask;
assert('`loadVersionsTask.perform()` must be called before calling `versionIdsByDate`', last != null);
let versions = last?.value ?? [];
return versions
.slice()
.sort(compareVersionByDate)
.map(v => v.id);
}

@cached get firstVersionId() {
return this.versionIdsByDate.at(-1);
}

@cached get versionsObj() {
let versions = this.versions.toArray() ?? [];
return Object.fromEntries(versions.map(v => [v.id, v]));
let { last } = this.loadVersionsTask;
assert('`loadVersionsTask.perform()` must be called before calling `versionsObj`', last != null);
let versions = last?.value ?? [];
return Object.fromEntries(versions.slice().map(v => [v.id, v]));
}

@cached get releaseTrackSet() {
Expand Down Expand Up @@ -117,6 +129,10 @@ export default class Crate extends Model {
let [teams, users] = await Promise.all([this.owner_team, this.owner_user]);
return [...(teams ?? []), ...(users ?? [])];
});

loadVersionsTask = task(async () => {
return (await this.versions) ?? [];
});
}

function compareVersionBySemver(a, b) {
Expand Down
12 changes: 12 additions & 0 deletions app/routes/crate/versions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import Route from '@ember/routing/route';
import { waitForPromise } from '@ember/test-waiters';

export default class VersionsRoute extends Route {
setupController(controller) {
super.setupController(...arguments);
let crate = this.modelFor('crate');
controller.set('crate', crate);
// TODO: Add error handling
waitForPromise(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.

I'm wondering if it makes more sense to call this task in the model() or at least afterModel() hooks, so that the data is fully loaded before the route displays anything. I don't know why it was originally implemented this way.

I guess this is a non-blocking concern though, since the code previously used the same pattern 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although implementing it this way could potentially be more complex, it would also be more responsive, as it could display all other elements (layout, crate header) before the versions are ready (a loading indicator for this hasn't been implemented yet).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But yeah, I mostly implemented it following the previous pattern. But I guess putting this either in model() or afterModel() should also work for non-blocking purposes, just note not to await for it.

}
}
6 changes: 3 additions & 3 deletions tests/components/version-list-row-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ module('Component | VersionList::Row', function (hooks) {

let store = this.owner.lookup('service:store');
let crateRecord = await store.findRecord('crate', crate.name);
let versions = (await crateRecord.versions).slice();
let versions = (await crateRecord.loadVersionsTask.perform()).slice();
await crateRecord.loadOwnerUserTask.perform();
this.firstVersion = versions[0];
this.secondVersion = versions[1];
Expand All @@ -39,7 +39,7 @@ module('Component | VersionList::Row', function (hooks) {

let store = this.owner.lookup('service:store');
let crateRecord = await store.findRecord('crate', crate.name);
this.version = (await crateRecord.versions).slice()[0];
this.version = (await crateRecord.loadVersionsTask.perform()).slice()[0];
await crateRecord.loadOwnerUserTask.perform();

await render(hbs`<VersionList::Row @version={{this.version}} />`);
Expand Down Expand Up @@ -73,7 +73,7 @@ module('Component | VersionList::Row', function (hooks) {

let store = this.owner.lookup('service:store');
let crateRecord = await store.findRecord('crate', crate.name);
let versions = (await crateRecord.versions).slice();
let versions = (await crateRecord.loadVersionsTask.perform()).slice();
await crateRecord.loadOwnerUserTask.perform();
this.firstVersion = versions[0];
this.secondVersion = versions[1];
Expand Down
8 changes: 4 additions & 4 deletions tests/models/version-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ module('Model | Version', function (hooks) {
}

let crateRecord = await this.store.findRecord('crate', crate.name);
let versions = (await crateRecord.versions).slice();
let versions = (await crateRecord.loadVersionsTask.perform()).slice();

assert.deepEqual(
versions.map(it => ({ num: it.num, isHighestOfReleaseTrack: it.isHighestOfReleaseTrack })),
Expand Down Expand Up @@ -198,7 +198,7 @@ module('Model | Version', function (hooks) {
this.server.create('version', { crate, num: '0.4.2', yanked: true });

let crateRecord = await this.store.findRecord('crate', crate.name);
let versions = (await crateRecord.versions).slice();
let versions = (await crateRecord.loadVersionsTask.perform()).slice();

assert.deepEqual(
versions.map(it => ({ num: it.num, isHighestOfReleaseTrack: it.isHighestOfReleaseTrack })),
Expand All @@ -216,7 +216,7 @@ module('Model | Version', function (hooks) {
this.server.create('version', { crate, num: '0.4.1' });

let crateRecord = await this.store.findRecord('crate', crate.name);
let versions = (await crateRecord.versions).slice();
let versions = (await crateRecord.loadVersionsTask.perform()).slice();

assert.deepEqual(
versions.map(it => ({ num: it.num, isHighestOfReleaseTrack: it.isHighestOfReleaseTrack })),
Expand All @@ -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.versions).slice();
versions = (await crateRecord.loadVersionsTask.perform()).slice();

assert.deepEqual(
versions.map(it => ({ num: it.num, isHighestOfReleaseTrack: it.isHighestOfReleaseTrack })),
Expand Down