storybookjs / storybook

Storybook is the industry standard workshop for building, documenting, and testing UI components in isolation
https://storybook.js.org
MIT License
84.12k stars 9.25k forks source link

Storybook Preact JSX Typings Default to React #8686

Closed sjmtan closed 4 years ago

sjmtan commented 4 years ago

Describe the bug TS is throwing an error to compile in a mixed React/Preact (private) repo. I've nailed it down to the following line:

export type StoryFnPreactReturnType = string | Node | JSX.Element;

Apologies in advance for some lack of TS terminology, but the JSX namespace exists under React and Preact, which leads to TS confusion. An error is thrown because it expects stories to return a JSX.Element, but without specifying that this should be a preact.JSX.Element, it will resolve to React.JSX.Element (and there are some interface differences).

My proposal is the following:

export type StoryFnPreactReturnType = string | Node | preact.JSX.Element;

The change here would be to declare the namespace of JSX.Element more clearly.

I did attempt to make a PR for this but was unable to push my branch up to origin. Do I need to request access to be a contributor?

To Reproduce Created a story with Preact, and JSX pragma at the top of the file with the stories.

Expected behavior No error.

System: Environment Info:

System: OS: macOS Mojave 10.14.5 CPU: (12) x64 Intel(R) Core(TM) i7-8850H CPU @ 2.60GHz Binaries: Node: 8.16.0 - ~/.nvm/versions/node/v8.16.0/bin/node Yarn: 1.19.1 - ~/dev/storm/node_modules/.bin/yarn npm: 6.12.1 - ~/.nvm/versions/node/v8.16.0/bin/npm Browsers: Chrome: 78.0.3904.87 Firefox: 69.0.3 Safari: 12.1.1 npmPackages: @storybook/addon-actions: ^5.2.5 => 5.2.5 @storybook/addon-backgrounds: ^5.2.5 => 5.2.5 @storybook/addon-centered: ^5.2.5 => 5.2.5 @storybook/addon-knobs: ^5.2.5 => 5.2.5 @storybook/addon-options: ^5.2.5 => 5.2.5 @storybook/addon-viewport: ^5.2.5 => 5.2.5 @storybook/cli: ^5.2.5 => 5.2.5 @storybook/preact: ^5.2.5 => 5.2.5 @storybook/react: ^5.2.5 => 5.2.5

shilman commented 4 years ago

@shannontan-bolt thanks for wanting to contribute--your proposed change looks reasonable to me!

you can fork the storybook repo, push your changes to your fork, and issue a PR from your fork to the main repo. 👍

stale[bot] commented 4 years ago

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

ultrafez commented 4 years ago

@shannontan-bolt did you raise a PR with your fix for this issue? This is something we're experiencing too that we're currently having to work around in a fairly ugly way.

stale[bot] commented 4 years ago

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

shilman commented 4 years ago

Yay!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.0.0-alpha.0 containing PR #9123 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

sjmtan commented 4 years ago

Apologies everyone! Somehow, I missed all the notifications for this hence no response.

Thanks for getting the fix in!

maxmilton commented 4 years ago

@shilman thanks for the release, great to see some movement here!

I am concerned that it's in a major alpha version. This means we'll need to run an unstable storybook just to get the fix. Not sure it's something that warrants a major/breaking change.

Could we get the change added to v5 in a patch/bugfix version instead?

shilman commented 4 years ago

@MaxMilton will do!