Skip to content

routes/crate: Fetch crate when peeked crate lacks included values #10666

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

Merged
merged 1 commit into from
Feb 24, 2025
Merged

routes/crate: Fetch crate when peeked crate lacks included values #10666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 9 additions & 1 deletion app/routes/crate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,15 @@ export default class CrateRoute extends Route {
let crateName = params.crate_id;

try {
return this.store.peekRecord('crate', crateName) || (await this.store.queryRecord('crate', { name: crateName }));
// We would like the peeked crate to include information (such as keywords) for further
// processing. Currently, we determine this by checking if associated versions exist,
// as default_version is included in the queryRecord call.
// See: https://github.com/rust-lang/crates.io/issues/10663
let crate = this.store.peekRecord('crate', crateName);
if (!crate || crate.hasMany('versions').value() == null) {
crate = await this.store.queryRecord('crate', { name: crateName });
}
return crate;
} catch (error) {
if (error instanceof NotFoundError) {
let title = `${crateName}: Crate not found`;
Expand Down
10 changes: 10 additions & 0 deletions e2e/acceptance/crate.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,4 +245,14 @@ test.describe('Acceptance | crate page', { tag: '@acceptance' }, () => {

await expect(page).toHaveURL('/crates/nanomsg/settings');
});

test('keywords are shown when navigating from search', async ({ page, msw }) => {
loadFixtures(msw.db);

await page.goto('/search?q=nanomsg');
await page.getByRole('link', { name: 'nanomsg', exact: true }).click();

await expect(page).toHaveURL('/crates/nanomsg');
await expect(page.locator('[data-test-keyword]')).toBeVisible();
});
Comment on lines +249 to +257
Copy link
Contributor Author

@eth3lbert eth3lbert Feb 23, 2025

Choose a reason for hiding this comment

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

There's another case that would cause the same issue: navigating from dependencies that have more than one dependency (as this will query with ?ids=). We would need to expand the fixtures to have at least two dependencies. Let me know if we'd like to include this as well.

Copy link
Member

@Turbo87 Turbo87 Feb 24, 2025

Choose a reason for hiding this comment

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

probably fine as-is. if anything, IMHO we shouldn't extend the hardcoded fixtures and instead setup a dedicated scenario for the test. I would prefer to get rid of the fixtures in general.

});
10 changes: 10 additions & 0 deletions tests/acceptance/crate-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,4 +249,14 @@ module('Acceptance | crate page', function (hooks) {

assert.strictEqual(currentURL(), '/crates/nanomsg/settings');
});

test('keywords are shown when navigating from search', async function (assert) {
loadFixtures(this.db);

await visit('/search?q=nanomsg');
await click('[data-test-crate-link]');

assert.strictEqual(currentURL(), '/crates/nanomsg');
assert.dom('[data-test-keyword]').exists();
});
});
Loading