svelte: Load crate owners from API #12767
Conversation
Turbo87
force-pushed
the
svelte-load-owners
branch
from
12c8c8b to
8d25d04
Compare
| crate: Crate; | ||
| version: Version; | ||
| keywords?: Keyword[]; | ||
| owners?: Owner[]; |
There was a problem hiding this comment.
I'm quite curious: why does owners need to be optional? Is it for the loading stage? I don't think crates can be without owners, right?
There was a problem hiding this comment.
good point. there is no reason to make it optional since both usages of the component supply the prop. I've changed it to non-optional. 👍
There was a problem hiding this comment.
Alright, wonderful. Would the same apply to the requestedVersion prop as well?
There was a problem hiding this comment.
Would the same apply to the requestedVersion prop as well?
nope, because that one isn't used by https://github.com/rust-lang/crates.io/blob/main/svelte/src/routes/crates/%5Bcrate_id%5D/%2Bpage.svelte#L11-L17, only by the route that includes the version number.
There was a problem hiding this comment.
This change would force us to load it before rendering the page. However, we could potentially treat it as a loading state when the owners list is empty.
Turbo87
force-pushed
the
svelte-load-owners
branch
3 times, most recently
from
c9bce48 to
dfb216f
Compare
| let crateName = params.crate_id; | ||
|
|
||
| let { crate, categories, keywords, defaultVersion } = await loadCrate(client, crateName); | ||
| let [crateData, owners] = await Promise.all([loadCrate(client, crateName), loadOwners(client, crateName)]); |
There was a problem hiding this comment.
Not a blocker: I guess this makes the behavior slightly different from the Ember version, as we now need to wait for owners to load before we can render the page. However, I think it's fine to merge this if you want to migrate first and retrofit it later.
There was a problem hiding this comment.
yeah, that was a deliberate decision. we don't have any kind of loading state for the owners list, so it felt a bit strange to load that data async. do you think we should return ownersPromise instead?
There was a problem hiding this comment.
Yeah, I think using an ownerPromise or an empty list as a loading state would be better(if we want to avoid blocking).
There was a problem hiding this comment.
I've added a second commit that implements this. is this what you had in mind?
Turbo87
force-pushed
the
svelte-load-owners
branch
from
dfb216f to
b9f0b38
Compare
Turbo87
force-pushed
the
svelte-load-owners
branch
from
b9f0b38 to
cfb4ebe
Compare
4 participants
This resolves the two "load owners"
TODOcomments. The owners list is loaded inroutes/crates/[crate_id]/+layout.ts?so that it can be shared with all subpages. This will allow the crate header to only show the "Settings" tab if the authenticated user (still TODO) is part of the owners list, similar to the Ember.js app.Related