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 all commits
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
13 changes: 2 additions & 11 deletions app/components/crate-header.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,30 +8,21 @@ export default class CrateHeader extends Component {
@service session;

@alias('loadKeywordsTask.last.value') keywords;
@alias('loadOwnerUserTask.last.value') ownerUser;

constructor() {
super(...arguments);

this.loadKeywordsTask.perform().catch(() => {
// ignore all errors and just don't display keywords if the request fails
});
this.loadOwnerUserTask.perform().catch(() => {
// ignore all errors and just don't display settings if the request fails
});
}

get isOwner() {
let ownerUser = this.ownerUser ?? [];
let currentUserId = this.session.currentUser?.id;
return ownerUser.some(({ id }) => id === currentUserId);
let userId = this.session.currentUser?.id;
return this.args.crate?.hasOwnerUser(userId) ?? false;
}

loadKeywordsTask = task(async () => {
return (await this.args.crate?.keywords) ?? [];
});

loadOwnerUserTask = task(async () => {
return (await this.args.crate?.owner_user) ?? [];
});
}
2 changes: 1 addition & 1 deletion app/components/crate-sidebar.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@
</div>

{{#unless @crate.categories.isPending}}
{{#if @crate.categories}}
{{#if @crate.categories.length}}
<div>
<h2 local-class="heading">Categories</h2>
<ul local-class="categories">
Expand Down
3 changes: 2 additions & 1 deletion app/components/version-list/row.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ export default class VersionRow extends Component {
}

get isOwner() {
return this.args.version.crate?.owner_user?.findBy('id', this.session.currentUser?.id);
let userId = this.session.currentUser?.id;
return this.args.version.crate.hasOwnerUser(userId);
}

@action setFocused(value) {
Expand Down
13 changes: 11 additions & 2 deletions app/controllers/crate/settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,12 @@ export default class CrateSettingsController extends Controller {

if (owner.kind === 'team') {
this.notifications.success(`Team ${owner.get('display_name')} removed as crate owner`);
this.crate.owner_team.removeObject(owner);
let owner_team = await this.crate.owner_team;
removeOwner(owner_team, owner);
} else {
this.notifications.success(`User ${owner.get('login')} removed as crate owner`);
this.crate.owner_user.removeObject(owner);
let owner_user = await this.crate.owner_user;
removeOwner(owner_user, owner);
}
} catch (error) {
let subject = owner.kind === 'team' ? `team ${owner.get('display_name')}` : `user ${owner.get('login')}`;
Expand All @@ -54,3 +56,10 @@ export default class CrateSettingsController extends Controller {
}
});
}

function removeOwner(owners, target) {
let idx = owners.indexOf(target);
if (idx !== -1) {
owners.splice(idx, 1);
}
}
11 changes: 9 additions & 2 deletions app/controllers/crate/version.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,14 @@ export default class CrateVersionController extends Controller {
this.stackedGraph = false;
}

@alias('downloadsContext.version_downloads.content') downloads;
@alias('loadDownloadsTask.last.value') downloads;
@alias('model.crate') crate;
@alias('model.requestedVersion') requestedVersion;
@alias('model.version') currentVersion;

get isOwner() {
return this.crate.owner_user.findBy('id', this.session.currentUser?.id);
let userId = this.session.currentUser?.id;
return this.crate.hasOwnerUser(userId);
}

@alias('loadReadmeTask.last.value') readme;
Expand Down Expand Up @@ -62,4 +63,10 @@ export default class CrateVersionController extends Controller {

return readme;
});

// This task would be `perform()` in setupController
loadDownloadsTask = task(async () => {
let downloads = await this.downloadsContext.version_downloads;
return downloads;
});
}
51 changes: 42 additions & 9 deletions app/models/crate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
import Model, { attr, hasMany } from '@ember-data/model';
import { assert } from '@ember/debug';
import { waitForPromise } from '@ember/test-waiters';
import { cached } from '@glimmer/tracking';

import { apiAction } from '@mainmatter/ember-api-actions';
import { task } from 'ember-concurrency';

export default class Crate extends Model {
@attr name;
Expand Down Expand Up @@ -35,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 All @@ -65,10 +79,16 @@ export default class Crate extends Model {
return new Set(map.values());
}

hasOwnerUser(userId) {
let { last } = this.loadOwnerUserTask;
assert('`loadOwnerUserTask.perform()` must be called before calling `hasOwnerUser()`', last != null);
return (last?.value ?? []).some(({ id }) => id === userId);
}

get owners() {
let teams = this.owner_team.toArray() ?? [];
let users = this.owner_user.toArray() ?? [];
return [...teams, ...users];
let { last } = this.loadOwnersTask;
assert('`loadOwnersTask.perform()` must be called before accessing `owners`', last != null);
return last?.value ?? [];
}

async follow() {
Expand Down Expand Up @@ -100,6 +120,19 @@ export default class Crate extends Model {
throw response;
}
}

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

loadOwnersTask = task(async () => {
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
4 changes: 4 additions & 0 deletions app/routes/crate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
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';

export default class CrateRoute extends Route {
@service headData;
Expand All @@ -26,6 +27,9 @@ export default class CrateRoute extends Route {
setupController(controller, model) {
super.setupController(...arguments);
this.headData.crate = model;
waitForPromise(model.loadOwnerUserTask.perform()).catch(() => {
// ignore all errors if the request fails
});
}

resetController() {
Expand Down
8 changes: 8 additions & 0 deletions app/routes/crate/version.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,16 @@ export default class VersionRoute extends Route {
waitForPromise(controller.loadReadmeTask.perform()).catch(() => {
// ignored
});
waitForPromise(controller.loadDownloadsTask.perform()).catch(() => {
// ignored
});

let { crate, version } = model;

waitForPromise(crate.loadOwnersTask.perform()).catch(() => {
// ignored
});

if (!crate.documentation || crate.documentation.startsWith('https://docs.rs/')) {
version.loadDocsStatusTask.perform().catch(error => {
// report unexpected errors to Sentry and ignore `ajax()` errors
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.

}
}
12 changes: 8 additions & 4 deletions tests/components/crate-sidebar/playground-button-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ module('Component | CrateSidebar | Playground Button', function (hooks) {

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

await render(hbs`<CrateSidebar @crate={{this.crate}} @version={{this.version}} />`);
assert.dom('[data-test-playground-button]').doesNotExist();
Expand All @@ -45,7 +46,8 @@ module('Component | CrateSidebar | Playground Button', function (hooks) {

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

let expectedHref =
'https://play.rust-lang.org/?edition=2021&code=use%20aho_corasick%3B%0A%0Afn%20main()%20%7B%0A%20%20%20%20%2F%2F%20try%20using%20the%20%60aho_corasick%60%20crate%20here%0A%7D';
Expand All @@ -63,7 +65,8 @@ module('Component | CrateSidebar | Playground Button', function (hooks) {

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

render(hbs`<CrateSidebar @crate={{this.crate}} @version={{this.version}} />`);
await waitFor('[data-test-owners]');
Expand All @@ -81,7 +84,8 @@ module('Component | CrateSidebar | Playground Button', function (hooks) {

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

await render(hbs`<CrateSidebar @crate={{this.crate}} @version={{this.version}} />`);
assert.dom('[data-test-playground-button]').doesNotExist();
Expand Down
9 changes: 6 additions & 3 deletions tests/components/crate-sidebar/toml-snippet-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ module('Component | CrateSidebar | toml snippet', function (hooks) {

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

await render(hbs`<CrateSidebar @crate={{this.crate}} @version={{this.version}} />`);
assert.dom('[title="Copy command to clipboard"]').exists().hasText('cargo add foo');
Expand All @@ -34,7 +35,8 @@ module('Component | CrateSidebar | toml snippet', function (hooks) {

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

await render(hbs`<CrateSidebar @crate={{this.crate}} @version={{this.version}} />`);
assert.dom('[title="Copy Cargo.toml snippet to clipboard"]').exists().hasText('foo = "1.0.0"');
Expand All @@ -46,7 +48,8 @@ module('Component | CrateSidebar | toml snippet', function (hooks) {

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

await render(hbs`<CrateSidebar @crate={{this.crate}} @version={{this.version}} />`);
assert.dom('[title="Copy Cargo.toml snippet to clipboard"]').exists().hasText('foo = "1.0.0-alpha"');
Expand Down
15 changes: 5 additions & 10 deletions tests/components/owners-list-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ module('Component | OwnersList', function (hooks) {

let store = this.owner.lookup('service:store');
this.crate = await store.findRecord('crate', crate.name);
await this.crate.hasMany('owner_team').load();
await this.crate.hasMany('owner_user').load();
await this.crate.loadOwnersTask.perform();

await render(hbs`<OwnersList @owners={{this.crate.owners}} />`);
assert.dom('[data-test-owners="detailed"]').exists();
Expand All @@ -44,8 +43,7 @@ module('Component | OwnersList', function (hooks) {

let store = this.owner.lookup('service:store');
this.crate = await store.findRecord('crate', crate.name);
await this.crate.hasMany('owner_team').load();
await this.crate.hasMany('owner_user').load();
await this.crate.loadOwnersTask.perform();

await render(hbs`<OwnersList @owners={{this.crate.owners}} />`);
assert.dom('[data-test-owners="detailed"]').exists();
Expand All @@ -70,8 +68,7 @@ module('Component | OwnersList', function (hooks) {

let store = this.owner.lookup('service:store');
this.crate = await store.findRecord('crate', crate.name);
await this.crate.hasMany('owner_team').load();
await this.crate.hasMany('owner_user').load();
await this.crate.loadOwnersTask.perform();

await render(hbs`<OwnersList @owners={{this.crate.owners}} />`);
assert.dom('[data-test-owners="detailed"]').exists();
Expand All @@ -93,8 +90,7 @@ module('Component | OwnersList', function (hooks) {

let store = this.owner.lookup('service:store');
this.crate = await store.findRecord('crate', crate.name);
await this.crate.hasMany('owner_team').load();
await this.crate.hasMany('owner_user').load();
await this.crate.loadOwnersTask.perform();

await render(hbs`<OwnersList @owners={{this.crate.owners}} />`);
assert.dom('[data-test-owners="basic"]').exists();
Expand All @@ -120,8 +116,7 @@ module('Component | OwnersList', function (hooks) {

let store = this.owner.lookup('service:store');
this.crate = await store.findRecord('crate', crate.name);
await this.crate.hasMany('owner_team').load();
await this.crate.hasMany('owner_user').load();
await this.crate.loadOwnersTask.perform();

await render(hbs`<OwnersList @owners={{this.crate.owners}} />`);
assert.dom('[data-test-owners="detailed"]').exists();
Expand Down
9 changes: 6 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,8 @@ 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 @@ -38,7 +39,8 @@ 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}} />`);
assert.dom('[data-test-release-track]').hasText('?');
Expand Down Expand Up @@ -71,7 +73,8 @@ 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];
this.thirdVersion = versions[2];
Expand Down
Loading
Loading