-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add asset management functions and hooks #656
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
cngonzalez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like having all these helper hooks! I'm not sure I fully understand why there's a need for a separate store, though. Can we treat image assets like documents?
Also added some comments on maybe using our existing tooling to keep this simple.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
cngonzalez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nits but a larger comment:
I really like the addition of the utility hooks but I'm a bit confused about the implementation of useAssets and the underlying getAssetsState et al. I've got two main questions:
- We're using the
StateSourcetype but are we actually using a state at any point? (Or the associated subscription / unsubsciption logic). I couldn't see it, but I could be wrong. Should we, to be consistent? useAssetsdiverges a bit from the rest of our codebase; I think it's returning whole documents. Should it conform more closely touseDocumentsand return handles (we might even be able to pass the buck togetDocumentsStateand have simpler logic)? And then we can implement auseAssetthat returns the full document that can then be a full doc? Or is that annoying in practice?
| // Bump this to force a re-fetch of assets | ||
| const [refresh, setRefresh] = useState(0) | ||
| const [isUploading, setIsUploading] = useState(false) | ||
| const requeryTimeoutsRef = useRef<number[]>([]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious if we still need this after the move to queryStore? Or if this is logic we could already bake in to our assets store? My personal preference is that our usage of hooks in Kitchensink is pretty "vanilla" (mostly for e2e tests to be accurate)
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable simple-import-sort/exports */ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why disable here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, good catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we remove this? i think we want this rule in this file especially
| * Runtime validator and type guard for image asset IDs | ||
| * @public | ||
| */ | ||
| export function isImageAssetId(value: string): value is ImageAssetId { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use asset-utils for this as well? https://github.com/sanity-io/asset-utils/blob/bb980f9cdb10922e7c5a13eacab564581c6120d6/src/asserters.ts#L99
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that importing from the monorepo would cause a dependency loop, so I'm hesitant to add it here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which monorepo do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we got an answer to this conversation about importing from asset-utils to keep consistent with the rest of the org.
| /** | ||
| * Identifies a specific asset within a Sanity project/dataset. | ||
| * Includes optional `projectId`/`dataset` and the required `assetId` (asset document `_id`). | ||
| * Useful for cross-instance asset operations without relying on the active ResourceProvider. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I fully understand this comment -- what is meant by cross-instance? I'm also a little wary of the mental model of "this is a document but the _id has a different parameter name"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cross-instance is across projects and datasets. This change is from Cole's suggestion here: https://sanity-io.slack.com/archives/C07RS3HV1K9/p1761929608902129?thread_ts=1761841748.625049&cid=C07RS3HV1K9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the addition of it. It makes it much easier to use assets from difference resources pretty easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we are using the active resource provider -- the queryStore is tied to it (that could be my fault -- I pushed you in that direction). I'm just confused on how this behavior differs from other hooks, which I think will also allow a projectId/dataset param to find the right instance.
|
I was multitasking and didn't write carefully 🤦 What I meant was, we're not using the Zustand store state as we do with other utilities. Stores like the I actually don't think that assets should have a store or a state; to me, they're just documents. Which brings me to this:
I'm mostly wary of having diverging implementations for things that are just documents. The I would love if we could scale all the efficiencies we worked for (regarding document querying) to the way we work with assets, and I think the easiest way is if we treat the To be clear, I'm very much in favor of |
|
@cngonzalez I think I have refactored to your suggestions. Please let me know if this is what you had in mind. Thanks |
cngonzalez
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if you want to chat or if I'm misunderstanding things! Sorry to drag this out.
| @@ -1,3 +1,4 @@ | |||
| /* eslint-disable simple-import-sort/exports */ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we remove this? i think we want this rule in this file especially
| * Runtime validator and type guard for image asset IDs | ||
| * @public | ||
| */ | ||
| export function isImageAssetId(value: string): value is ImageAssetId { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if we got an answer to this conversation about importing from asset-utils to keep consistent with the rest of the org.
| * Options for querying assets | ||
| * @public | ||
| */ | ||
| export interface AssetQueryOptions< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sorry -- it's been a while and I didn't realize you'd made changes based on my feedback. I still feel pretty strongly that assets are just documents and we should maybe update / share useDocuments rather than having the complexity of a dedicated assets store. I think it also might simplify usage -- I notice a lot of re-querying and other things in the Kitchensink example that don't feel very SDK (or maybe those things don't need to be there and I'm misunderstanding?)
It would also mean that our signatures for useAssets and useDocuments could easily be the same. If we could abstract out the "meat" of useDocuments, so that useAssets could return the full AssetDocumentBase, while useDocuments returns DocumentHandles, I think we'd have a lot of the same functionality and optimization and upkeep would be a lot easier.
I apologize, I thought we'd discussed this but maybe it was all in my head.
| @@ -2,29 +2,26 @@ | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to not make changes to the changelog in this PR
| TProjectId extends string = string, | ||
| > extends DatasetHandle<TDataset, TProjectId> { | ||
| assetId: string | ||
| mediaLibraryId: string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mark this beta? I'm a bit wary of exposing something that uses the mediaLibraryId explicitly as public, especially with upcoming sources work

Description
Added asset management capabilities to the SDK, including new hooks and utilities for working with assets. This PR introduces:
@sanity/sdk@sanity/sdk-reactfor asset operationsThe implementation provides a complete asset management solution with support for:
What to review
packages/core/src/assets/assets.tspackages/react/src/hooks/assets/apps/kitchensink-react/src/routes/AssetsRoute.tsxTesting
The implementation includes:
Fun gif