-
-
Notifications
You must be signed in to change notification settings - Fork 9.2k
feat(core): fail-safe global data fetching #7083
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
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-7083--docusaurus-2.netlify.app/ |
Size Change: +56 B (0%) Total Size: 805 kB
ℹ️ View Unchanged
|
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.
Mitigated feelings about this.
Is it an API design that users would easily understand?
|
||
export function useAllPluginInstancesData( | ||
pluginName: string, | ||
): GlobalData[string]; | ||
options?: UseDataOptions, | ||
): GlobalData[string] | undefined; |
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 guess it could be possible to use TS overloads to avoid TS users to need to use ! despite the failfast option?
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.
Technically yes, but... err... at least not quite useful for us
useAllPluginInstancesData(pluginName: string) | ||
function useAllPluginInstancesData( | ||
pluginName: string, | ||
options?: {failfast?: boolean}, |
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.
is it really an option that we want to expose publicly? 🤷♂️
usePluginData('docusaurus-plugin-content-docs', pluginId) as GlobalPluginData; | ||
usePluginData('docusaurus-plugin-content-docs', pluginId, { | ||
failfast: true, | ||
}) as GlobalPluginData; |
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.
probably not needed with TS overloads
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.
It's needed because we are casting it to the shape of doc plugin's global data instead of the generic unknown
global data. Theoretically we need to validate but in spirit of pragmatism and bundle size I didn't
So the point is that in the only place where we are using global data, we are using it in a fail-safe way anyways. I think most users would also appreciate a more fail-safe API and handle |
ok 👍 good enough |
Motivation
The docs plugin hook can't use fail-fast because "it's used on non-doc pages" (but I think that's probably not the case anymore). But anyways, to avoid refactor hazards I just added a
failfast
option so we can take advantage of theuseAllPluginInstancesData
hook.Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Build