-
Notifications
You must be signed in to change notification settings - Fork 975
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
Improve typing on generated storybook components #6226
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit a138f50. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 4 targetsSent with 💌 from NxCloud. |
✅ Deploy Preview for redwoodjs-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
Thank you so much for this @will-ks - was on our list but couldn't get to yet! |
I just realised there's also stories generated for pages, layouts etc, which should also be given the same treatment. I'll close this PR and reopen when I've fixed the other ones. |
Components, Pages, Layouts and Cells now have the changes. |
Thanks @will-ks! I'm not great with TS, but one question: should we use
// Post component
const Post = ({
title,
description,
}: {
title: string
description: string
}) => {
return (
<div>
<h2>{title}</h2>
<p>{description}</p>
</div>
)
}
export default Post With With |
TODO: add |
@jtoar Good find, does look like |
Thanks again @will-ks - Just added a commit to regenerate stories in our fixtures (which we use for smoke-tests). The changes are looking good to me, except for in Cells, as you've mentioned previously - essentially we can't spread the args because the type is The other thing we discussed was also whether its worth including the type |
@will-ks it certainly is, but it's not actually out yet right? Just alpha releases so far. So in the near future! |
@dac09 I tried using the loading utility type ( Here's an alternative: let's just not pass redwood/packages/cli/src/commands/generate/cell/templates/cell.tsx.template Lines 12 to 14 in 1718499
So this... export const loading: ComponentStory<typeof Loading> = (args) => {
return Loading ? <Loading {...args} /> : <></>
}
export const empty: ComponentStory<typeof Empty> = (args) => {
return Empty ? <Empty {...args} /> : <></>
} would become... export const loading = () => {
return Loading ? <Loading /> : <></>
}
export const empty = () => {
return Empty ? <Empty /> : <></>
} Thoughts? |
We actually run into the E.g., here's the import { Link, routes } from '@redwoodjs/router'
import { MetaTags } from '@redwoodjs/web'
const AboutPage = () => { // 👈 no props
return (
<p className="font-light">
This site was created to demonstrate my mastery of Redwood: Look on my
works, ye mighty, and despair!
</p>
)
}
export default AboutPage You'll see a red squiggle in the story file: import type { ComponentMeta, ComponentStory } from '@storybook/react'
import AboutPage from './AboutPage'
export const generated: ComponentStory<typeof AboutPage> = (args) => {
return <AboutPage {...args} /> // 👈 red squiggle here
}
export default {
title: 'Pages/AboutPage',
component: AboutPage,
} as ComponentMeta<typeof AboutPage> |
I'm not great with Storybook - we just recently started using it at work export const loading: ComponentStory<typeof Loading> = (args) => {
return Loading ? <Loading {...args} /> : <></>
} I was super confused about the spreading of So removing the args is a great improvement imho. |
export const loading = (args) => { | ||
return Loading ? <Loading {...args} /> : null | ||
export const loading: ComponentStory<typeof Loading> = (args) => { | ||
return Loading ? <Loading {...args} /> : <></> |
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.
Just a stylistic preference, but I'd probably do this:
return Loading ? <Loading {...args} /> : <></> | |
return <Loading {...args} /> || <></> |
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 at this again, do we actually need to handle this case? (Where Loading
isn't exported by the Cell right?) If Loading isn't defined, isn't TS going to complain when we import it further up?
import { Loading, Empty, Failure, Success } from './PostCell'
It looks like here we'll get something like Module '"./PostCell"' has no exported member 'Loading'.ts(2305)
.
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.
That would be even better! Just make sure it works the same with strict mode on/off
And JS doesn't care, right? So for the JS template we'd still have to do something like this, yeah?
Now that I'm more into it, it's a little more nuanced than I thought:
So it seems like @virtuoushub I know we added args in to get the "Show code" feature working. Is there another nuance here we're missing maybe? (It hooks up the controls add-on too right?) |
So maybe the thing to change here is to give generated components some dummy prop? That way we could keep the args spreading for components. And for pages we could generate the stories with args spreading if the page is generated for a path with parameters ( |
Not sure it'd be a solved problem, but doing that is definitely part of the solution here. I didn't plan to bring that one up again until we were on React18 though, because with React18 they improved the |
- cell -> loading / empty, don't pass args - component -> explain args - page -> only pass args to route params page
import ${pascalName}Page from './${pascalName}Page' | ||
|
||
export const generated: ComponentStory<typeof ${pascalName}Page> = (args) => { | ||
return <${pascalName}Page ${propValueParam} {...args} /> |
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.
Am I correct in thinking that propValueParam
is used to provide a default value, but you can then let {...args}
spread over it to render with a different value for that prop?
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.
Yeah that's right!
sb_args.mov
Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
This reverts commit 544718c.
fwiw - Yes, I think it has to do with the controls add-on. Outside of that I do not really know the ramifications for removing it out side of what you already listed. I have not dealt with TypeScript/Redwood in a little bit - Do we generate the component's props as well? I think we could have the generated stories adhere to the correct typings in strict mode if we do unless I am missing something. |
Currently we generate all components without props. We know most components will probably have props, but it's just really difficult to generate sensible default props, because it varies so much what props you'll want/need |
Makes sense, imho we might want to consider at least "boiler-plating" empty props since we are generating the components and we know most people will want props. This way we can punt on what sensible defaults are, but at least provide the scaffolding to allow a framework user to easily add in what is needed later. |
100% agree with that. And it's the route we're going to take when we upgrade to React 18, because then we can also start using the improved |
…-performance * 'main' of github.com:redwoodjs/redwood: (25 commits) chore(deps): update dependency @types/jsonwebtoken to v8.5.9 (redwoodjs#6309) chore(deps): update dependency @nhost/nhost-js to v1.4.10 (redwoodjs#6306) fix(deps): update dependency fastify to v4.5.3 (redwoodjs#6313) fix(deps): update jest monorepo to v29.0.1 (redwoodjs#6314) docs: fix typos (redwoodjs#6310) fix(deps): update dependency dotenv-webpack to v8.0.1 (redwoodjs#6301) chore(deps): update dependency @auth0/auth0-spa-js to v1.22.3 (redwoodjs#6299) Adds a Deploy Keys section to server doc (redwoodjs#6304) Improve typing on generated storybook components (redwoodjs#6226) Update yarn.lock (in docs) fix(deps): update dependency @graphql-codegen/cli to v2.11.7 (redwoodjs#6300) try reverting tough-cookie patch change (redwoodjs#6298) Update yarn.lock v2.2.3 fix(deps): update dependency @types/jest to v28.1.8 (redwoodjs#6294) remove testURL Update yarn.lock chore(deps): update dependency @playwright/test to v1.25.1 (redwoodjs#6283) fix(deps): update dependency @graphql-yoga/common to v2.12.8 (redwoodjs#6297) chore(deps): update yarn to v3.2.3 (redwoodjs#6284) ...
This PR fixes a couple of issues with the storybook stories generated for each component.
args
not being properly typed:This PR adds type assertion to the
generated
story, so theargs
parameter is now properly typed.This PR sets the
component
field in the story metadata. This means that the generated story will now show auto-generated controls for the component's props, and removes the warning: