-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Docs changes for on demand store #16009
Docs changes for on demand store #16009
Conversation
Nx Cloud ReportWe didn't find any information for the current pull request with the commit 9f799b2. Check the Nx Cloud Github Integration documentation for more information. Sent with 💌 from NxCloud. |
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.
Looking good!!
addons/docs/src/blocks/ArgsTable.tsx
Outdated
@@ -45,42 +44,46 @@ type ArgsTableProps = BaseProps | OfProps | ComponentsProps | StoryProps; | |||
|
|||
const useArgs = ( | |||
storyId: string, | |||
storyStore: StoryStore | |||
context: DocsContextProps<any> |
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 this be DocsContextProps<AnyFramework>
as below?
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 TFramework
be AnyFramework
by default? Would eliminate a lot of boilerplate
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 TFramework be AnyFramework by default? Would eliminate a lot of boilerplate
I think when you are associating two types (e.g. a function that takes a StoryContext
and produces a Decorator
, you need the type variable so you can guarantee that the two AnyFramework
are the same AnyFramework
.
The analogy would be to functions that take any
vs a typed variable:
// Doesn't matter what `var1` is
function isTruthy(var1: any) {
return !!var1;
}
// Doesn't matter what `var1` and `var2` are, but they need to be same
function isEqual<T>(var1: T, var2: T) {
return var1 === var2;
}
// This will give a type error
isEqual(1, '1');
// You could define `isEqual` in terms of `any` but it'd lose some type info
function isEqual2(var1: any, var2: any) {
return var1 === var2;
}
// This won't give a type error, which is less useful
isEqual2(1, '1');
That's my understanding of it anyway, I could totally be wrong, my TS-fu is still pretty recent.
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.
Oh, I totally missed what you mean here. You mean to change the various types to like DocsContextProps<TFramework extends AnyFramework = AnyFramework>
.
I think that's a good idea for DocsContextProps
, but doesn't really help in most other cases for the same reason I posted above. But we may as well default the type I guess.
Merge as part of #15871 |
Part of #15871
Telescoping on #16006
What I did
Docs changes to support on-demand store.
DocsContext
changed a bit (and there were a bunch of type changes)ArgsTable
andStory
need to have loading stateThese changes were mostly pretty mechanical.