Skip to content
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

CSF3 react templates for storybook init #19462

Closed
wants to merge 4 commits into from

Conversation

kasperpeulen
Copy link
Contributor

@kasperpeulen kasperpeulen commented Oct 12, 2022

Issue:

What I did

  • Upgrade TS to 4.9
  • Update react templates for storybook init to CSF3
  • I included the templates (except for aurelia) and test files in tsc, and fixed all the tsc errors.

Blocked by

How to test

I added some unit tests, but I didn't test it yet in an e2e kind of setup. Should I do this manually?

.vscode/settings.json Outdated Show resolved Hide resolved
@@ -23,6 +22,8 @@ const postinstallAddon = async (addonName: string, isOfficialAddon: boolean) =>
LEGACY_CONFIGS.forEach((config) => {
try {
const codemod = require.resolve(
// TODO: fix this before merging
// @ts-expect-error getPackageName undefined, so will always go to catch branch
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this, seems impossible for this code to work if getPackageName is not imported

Copy link
Member

@ndelangen ndelangen Oct 23, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this code has been broken for a while, I suspect some bad merge from the past removed the import.

import React from 'react';
import type { ComponentStoryFn, ComponentMeta } from '@storybook/react';

import type { Meta, StoryObj } from '@storybook/react';
import { Button } from './Button';

// More on default export: https://storybook.js.org/docs/react/writing-stories/introduction#default-export
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonniebigodes — I think this deserves a pass with eyes toward docs/DX. For example:

  1. Now that this comment is placed before const meta and not export default, should the comment say "More on meta". And should all the "default export" references in the docs be changed to "meta"?
  2. Do we need a comment about the default (implicit) render function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I agree. I think you are right, so I already changed the template to "More on meta"

  2. I think so. Or at least an example with an explicit render function.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kylegach and @kasperpeulen, regarding the implicit/explicit render function, I would suggest going with one of the examples having an explicit render function, for instance, the Secondary Button story, and add a comment above it, for example, something like:

// Button.stories.js|jsx|ts|tsx

// imports and other information goes here

/*
 *👇 CSF 3 render functions are a framework-specific feature to allow you control over how the component renders.
 * See https://storybook.js.org/docs/7.0/react/api/csf
 * to learn how to use render functions.
 */
export const Secondary = {
  render: (args) => (
    <Button {...args} />
  ),
  args: {
    primary: true,
    label: 'Button',
  },
}

And in the others with an implicit render function, we could go with something like:

// Button.stories.js|jsx|ts|tsx

// imports and other information goes here

// CSF 3 story using an implicit render function
export const Primary = {
  args: {
    primary: true,
    label: 'Button',
  },
}

Regarding the const meta usage instead of the default export a couple of items to address:
1- If this is the intended path for every framework, the TS snippets in use need updating, and I'll follow up with you @kasperpeulen on them.
2- Instead of // More on meta comment instead, we could go with something like

// Button.stories.js|jsx|ts|tsx

// imports go here

// More on how to set up Storybook stories at: https://storybook.js.org/docs/react/writing-stories/introduction#default-export
const meta = {
  title: 'Example/Button',
  component: Button,
  // More on argTypes: https://storybook.js.org/docs/react/api/argtypes
  argTypes: {
    backgroundColor: { control: 'color' },
  },
} satisfies Meta<typeof Button>;

export default meta;

The reasoning behind this is that leaving it only // More on meta , the wording might throw off new users that installing Storybook for the first time and have no knowledge of how it works.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me @jonniebigodes.

  1. I'm still figuring out the other frameworks, I think it will be the intended path for Vue 2/3 and Svelte. Not sure yet about Lit and Angular, as I think we can not improve the type safety there with satisfies. We could still use the same format to keep it consistent.

  2. That wording also sounds good to me, I will leave that decision to you guys :)

Comment on lines 12 to 14
};
satisfies;
Meta<typeof Button>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the format here right? I would've expected:

} satisfies Meta<typeof Button>;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wow, I think eslint fix did that :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So unfixed the fix :P But looks better now :D Eslint doesn't support TS 4.9 yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we just have to upgrade the eslint parser?

import type { Meta, StoryObj } from '@storybook/react';
import { Button } from './Button';

// More on default export: https://storybook.js.org/docs/react/writing-stories/introduction#default-export
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment also applies to the legacy files.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I separated the default export and meta as well for the "legacy" templates, for 2 reasons:

  • It is more typesafe than as, as as will swallow some errors for you.
  • It already moves people closer to the new format, so it is easier to migrate when they are ready to adopt TS 4.9.

@kasperpeulen
Copy link
Contributor Author

@kylegach Thanks for the review! This PR is a bit of a mess, as I stopped ignoring the templates (except for aurelia) and test files in tsc, and fixed all the tsc errors, and bumped TS to 4.9. But glad you could find the important files 😅


export default meta;

export const Primary: StoryObj<typeof Button> = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type Story = StoryObj<typeof Button>;

@yannbf yannbf marked this pull request as draft October 17, 2022 14:21
@yannbf yannbf changed the title WIP: CSF3 react templates for storybook init CSF3 react templates for storybook init Oct 17, 2022
@socket-security
Copy link

Socket Security Report

Dependency issues detected. If you merge this pull request, you will not be alerted to the instances of these issues again.

📜 New install scripts detected

A dependency change in this PR is introducing new install scripts to your install step.

Package Script field Location
@ampproject/toolbox-optimizer@2.6.0 (added) postinstall code/examples/external-docs/package.json via nextra-theme-docs@1.2.6, next-themes@0.0.8, next@9.5.5
Socket.dev scan summary
Issue Status
Did you mean? ✅ no new possible package typos
Install scripts ⚠️ 1 new install script detected
Telemetry ✅ no new telemetry
Troll package ✅ no new troll packages
Malware ✅ no new malware
Native code ✅ no new native modules
Bot Commands

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of package-name@version specifiers. e.g. @SocketSecurity ignore foo@1.0.0 bar@2.4.2

  • @SocketSecurity ignore @ampproject/toolbox-optimizer@2.6.0

⚠️ Please accept the latest app permissions to ensure bot commands work properly. Accept the new permissions here.

Powered by socket.dev

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kasperpeulen Please split the TS upgrade into its own PR. The CLI template changes can telescope off of it.

@@ -1,12 +1,19 @@
/* eslint-disable no-underscore-dangle */
import { StorybookConfig } from '@storybook/core-common/src';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will be broken when published to npm, because the src directory won't get published

@kasperpeulen kasperpeulen mentioned this pull request Oct 27, 2022
@kasperpeulen
Copy link
Contributor Author

@kylegach @jonniebigodes
I split this PR in 3, I guess for you guys this one is interesting:
#19665

@ndelangen ndelangen deleted the future/new-react-ts-templates branch November 28, 2022 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants