-
Notifications
You must be signed in to change notification settings - Fork 1
feat: require project ID for project-level fetchers #631
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
9ebc4e0 to
8d9b001
Compare
2cbcac0 to
8b5e9b8
Compare
8d9b001 to
606c73f
Compare
8b5e9b8 to
5c97111
Compare
606c73f to
65123ff
Compare
5c97111 to
0833fe0
Compare
0833fe0 to
39345d2
Compare
65123ff to
1ea9fc7
Compare
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.
Less sure about this one because it's technically a breaking change. I'm unclear on how vital it is to make this change now
| export const datasets = createFetcherStore({ | ||
| name: 'Datasets', | ||
| getKey: (instance, options?: ProjectHandle) => { | ||
| const projectId = options?.projectId ?? instance.config.projectId |
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.
How strongly do we feel about continuing checking the instance in this PR and come back to it later in a broader breaking change? There are plenty of other hooks that still use the instance at this point in the stack, and this will throw anyway with a configuration that's just a Media Library source (I think)
(I also think there are a number of issues that will crop up now if you try to make an SDK app with just a Media Library source)
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.
Yea, this came from me basically uncommenting the definition of instance.config.projectId and trying to get rid of them. I think we can workaround this even with a new "named sources" concept by basically having a helper function like this:
function getProjectId(sources) {
// Prioritize default source:
if ('projectId' in sources.default) return sources.default.projectId
// Loop over all sources, find the projectId, if there's only one => return it
// If there's multiple => Error
}I can look into introducing this separately. I will close this and rather rebase the rest on top of latest main.

Description
(Note: There should be no changes to the React package here.)
This changes the core functions for working with dataset/project to explicitly require a project ID and not look into the SanityInstance configuration. This is part of a future where we only have a single SanityInstance and instead every function can be invoked on arbitrary projectId/datasets without being configured explicitly.
What to review
Testing
Fun gif