storybookjs / storybook

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

Storybook v6.4 is breaking users using react-router v5 #16837

Closed rhuanbarreto closed 2 years ago

rhuanbarreto commented 2 years ago

As storybook v6.4 brings react-router v6, people using v5 face breaking changes for a minor version update of storybook, which breaks semver.

Temporary solution is changing the version to ~6.3.12 instead of ^6.13.12 in the package.json file and putting "@storybook/router": "~6.3.10", in the resolution parameter of the package.json

And this breaking change is due to the storybook-addon-outline referenced in @storybook/addons-essentials. Follow the yarn why command. This package doesn't follow the fixed version nature of other storybook packages.

=> Found "react-router@6.0.2"
info Has been hoisted to "react-router"
info Reasons this module exists
   - Hoisted from "@storybook#addon-essentials#storybook-addon-outline#@storybook#addons#@storybook#router#react-router"
   - Hoisted from "@storybook#addon-essentials#storybook-addon-outline#@storybook#addons#@storybook#router#react-router-dom#react-router"
shilman commented 2 years ago

@rhuanbarreto why is it a breaking change for storybook to upgrade one of its own dependencies? if your code uses react-router v5, can't it continue to do that alongside storybook?

rhuanbarreto commented 2 years ago

The problem is that even if I use ~ to keep my dependencies on v6.3.x, @storybook-addons-essentials will bump react-router to v6.

rhuanbarreto commented 2 years ago

And if I upgrade to 6.4, react-router is upgraded to v6 even though I have react-router-dom: "^5.0.0" in my package.json.

shilman commented 2 years ago

Do you have a dependency on react-router@5 in your app? you shouldn't be relying on storybook's version of react-router in your app, right? or am i misunderstanding?

bdefore commented 2 years ago

@shilman i'm also encountering this issue or a similar one. bumping to 6.4 storybook deps cause tsc typecheck errors around react-router. adding "react-router": "^5.1.2" to deps works around it, but i'm wary of adding deps that aren't explicitly imported in the project. perhaps the issue is for projects that use react-router-dom instead of react-router.

bdefore commented 2 years ago

as a side note, a bit disappointed react-router team released 6 a month ago before backward compatibility path is finalized https://github.com/remix-run/react-router/blob/main/docs/upgrading/v5.md

vinothvk commented 2 years ago

I am also facing this issue.

rhuanbarreto commented 2 years ago

I'm experiencing the same issue as @bdefore but I'm using react router v5 in my project. doing the resolution solved my case for now but many others will also experience the same because of the loose react-router dependency on the storybook-addon-outline package that doesn't lock the version of @storybook/addons and therefore brings the v3.4 with react-router v6

shilman commented 2 years ago

What happens if you switch to @storybook/addon-outline?

techhysahil commented 2 years ago

i am also facing the same problem even though react-router-dom package in my package.json point to 5.xx version. i was using useHistory hook from react-router v-5. But now as the react-router got updated to 6.xx ..... now it's does not have above hook support. It purposes a new method but i will have to change it at lot of places to fix my build.

bdefore commented 2 years ago

@techhysahil i believe that hook is available in react-router but not react-router-dom. if you're importing from react-router in your project without declaring as a dependency you're implicitly reliant on whatever version your dependencies bring in (in this case storybook upped to v6). if you haven't already try adding it per my comment above.

MhMadHamster commented 2 years ago

Do you have a dependency on react-router@5 in your app? you shouldn't be relying on storybook's version of react-router in your app, right? or am i misunderstanding?

To clarify: if user wants to use typescript with react-router-dom they will install @types/react-router-dom, which has a dependency on @types/react-router and imports types from @types/react-router, but typescript resolves packages from @types subfolder of node_modules only if it cant find corresponding folder in the ./node_modules/. Now, when user installs any package that has a dependency on react-router, they will get react-router folder in their node_modules and since react-router@6.x now comes with types, you don't need to install @types/react-router separately. But if you are still using @types/react-router-dom, when resolving types for react-router typescript will find react-router/index.d.ts in root node_modules and use it as a type definition, hence people are getting the error about x not found in TS, because essentially types from react-router@6.x version are being used for @types/react-router-dom@5.x. This is obviously not an issue with storybook, just wanted to clarify why it happens when people updating to latest storybook version.

shilman commented 2 years ago

@MhMadHamster thanks so much for the clarification -- that's really tricky. the wonderful JS ecosystem strikes again! any idea what we can do to mitigate this sticky situation???

ghost commented 2 years ago

Having the same issue here as well.

MhMadHamster commented 2 years ago

@shilman I don't think there is a "good" solution, but there are a few options (none of them involve changing anything in storybook):

As for storybook, as I said, I don't think that's the issue within storybook, so I haven't really given a thought to how it can be mitigated from storybook side.

rhuanbarreto commented 2 years ago

The package addon-essentials removed the need of storybook-addon-outline in 6.4.7 and then the problem is solved. Thanks @shilman!

shilman commented 2 years ago

@rhuanbarreto That's great news! Seems like there's still something funny going on, but I'll wait and see how this issue evolves 🙈

ghost commented 2 years ago

Hey @rhuanbarreto , looks like essentials in 6.4.7 still requires @storybook/addon-outline: 6.4.7

ghost commented 2 years ago

Additionally to that, I now see that "storybook@addon-essentials@6.3.12" requires "storybook-addon-outline@^1.4.1" which requires:

    "@storybook/addons": ^6.3.0
    "@storybook/api": ^6.3.0
    "@storybook/components": ^6.3.0
    "@storybook/core-events": ^6.3.0

which resolves into 6.4.7

shilman commented 2 years ago

@winkerVSbecks can you please update storybook-addon-outline dependencies to ~6.3.0 and publish a new patch release? we didn't move from storybook-addon-outline until 6.4, so currently all 6.3 users might, under some set of conditions, get 6.4 dependencies pulled in per ☝️

floyd-may commented 2 years ago

The thing that I'm not seeing having been addressed above is that @storybook/router added dependencies for react-router and react-router-dom from 6.3.x to 6.4.x. Here's the situation of transitive dependencies at 6.3.x - at least for me (output from npm ls @storybook/router):

+-- @storybook/addon-essentials@6.3.12
| +-- @storybook/addon-docs@6.3.12
| | `-- @storybook/builder-webpack4@6.3.12
| |   +-- @storybook/router@6.3.12 deduped
| |   `-- @storybook/ui@6.3.12
| |     `-- @storybook/router@6.3.12 deduped
| `-- @storybook/api@6.3.12
|   `-- @storybook/router@6.3.12 deduped
+-- @storybook/addons@6.3.12
| `-- @storybook/router@6.3.12 deduped
`-- @storybook/router@6.3.12

Now with those new deps on react-router and react-router-dom, all these packages transitively depend on them at version 6.x. Clearly @storybook/router is heavily reused within the storybook packages. Other libs have solved this via peerDependencies. Yes, it means that @storybook/router et al would need to support multiple versions of react-router and friends, but it definitely saves us end-users from getting broken on minor upgrades. Is a peerDependencies solution (similar to what's already being done for react) worth considering here?

alexanderkho commented 2 years ago

aside from manually fixing the version of react-router in package.json are there any other fixes for this currently?

gk4m commented 2 years ago

I am also facing this issue, any updates on that?

ml-devninja commented 2 years ago

I have found this solution: https://github.com/gvaldambrini/storybook-router/issues/53#issuecomment-962399619

Works like a charm for me

bnussman commented 2 years ago

All of the proposed solutions are kind of wack... For repos that still use react-router-dom v5 Storybook's use of v6 causes a lot of compilation errors for that repo due to reasons @MhMadHamster explained. Is the solution to wait for the v5 / v6 backwards compatibility from the React Router team?

ghost commented 2 years ago

@ml-devninja how is that a solution to the issue? the problem is the dependency being updated in the project, not storybook specific.

israelKusayev commented 2 years ago

@shilman Are there any plans to fix it? I'm pretty sure a lot of people will not upgrade to version 6 of react-router anytime soon

react-router even recommends not to upgrade the version until they'll release a backward-compatible version

image

Thanks.

shilman commented 2 years ago

@israelKusayev do you have any proposals?

israelKusayev commented 2 years ago

@shilman Why do we even need react-router in storybook? if it's for some specific plugins maybe we can opt them out if we don't use them

shilman commented 2 years ago

@israelKusayev storybook's own UI uses it

f1lander commented 2 years ago

@rhuanbarreto @shilman how do you fix this through the resolution? this is the error that I have image

"react-router-dom": "5.2.0",

"@storybook/addon-actions": "^6.4.9",
"@storybook/addon-essentials": "^6.4.9",
"@storybook/addon-links": "^6.4.9",
"@storybook/builder-webpack5": "^6.4.9",
"@storybook/manager-webpack5": "^6.4.9",
"@storybook/node-logger": "^6.4.9",
"@storybook/preset-create-react-app": "^3.2.0",
"@storybook/react": "^6.4.9",
rhuanbarreto commented 2 years ago

The dependency problem was solved but now as @MhMadHamster said, typescript is still complaining.

f1lander commented 2 years ago

The dependency problem was solved but now as @MhMadHamster said, typescript is still complaining.

How this was solved? so that error that I'm having si regarding to typescript? you mentioned the resolutions if there is any possibility of some hack to get it working

renomateo commented 2 years ago

How this was solved?

I just did options 1 & 4 of MhMadHamster's comment and it appears to be working. Although adding paths to tsconfig.json doesn't seem to have any affect.

rhuanbarreto commented 2 years ago

For me it wasn't solved. If you clean node_modules then you keep getting the typing errors. For point 4, Create React App doesn't allow paths in tsconfig.json.

f1lander commented 2 years ago

@rhuanbarreto I don't know what I'm doing wrong, I'm using craco 6.4.3, and still having the same error, could you help me to verify what I'm doing wrong? this is the PR open https://github.com/coordinape/coordinape/pull/374/commits

michaeljsalo commented 2 years ago

I would just like to make clear this issue isn't limited to users of React Router. My team has no versions of react-router in our dependency tree at all. When upgrading to Storybook 6.4.x, react-router suddenly appears, and causes our app to fail to build, as it throws typecheck errors.

shilman commented 2 years ago

FYI we're working on a fix for this, hopefully arriving in the next few days 🤞

floyd-may commented 2 years ago

@shilman Much appreciated! Once the fix is ready I'll report back here with the results of testing on my project. Thank you!

floyd-may commented 2 years ago

apologies - I just learned that I've got to move to react-router v6 ASAP due to a bug that can't/won't be backported to v5. I won't be able to give you that testing report after all 😞

rhuanbarreto commented 2 years ago

keep it up @shilman! I will test it once it's done! Will this come to 6.4 or only 6.5?

shilman commented 2 years ago

Boo-yah!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.5.0-alpha.32 containing PR #17294 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb upgrade --prerelease

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

rhuanbarreto commented 2 years ago

Is there a way to backport it once 6.5 is still a little distant?

shilman commented 2 years ago

@rhuanbarreto Unfortunately this is too big a change to backport to 6.4. I'd recommend upgrading to 6.5 if you're hitting this issue--I don't anticipate any big destabilizing changes after this week, though there will be lots of small improvements before the final release.

rhuanbarreto commented 2 years ago

Then I will need to wait until 6.5 gets better stability. For now we need to live with the problem and ignore it in TS.

f1lander commented 2 years ago

We just update our react-router to 6 to avoid this issue but thanks a lot @shilman for all the efforts

DavidDworetzky commented 2 years ago

@shilman Thanks for the fix! I was running into the react-router-dom typescript issues on 6.4, found this thread and the alpha fix and it seems to work.

AngeloGiurano commented 2 years ago

@shilman I've tried to upgrade to the 6.5.0-alpha.34 but I see the following TS error:

Could not find a declaration file for module 'history'.

Is that expected? Any solution for that?

shilman commented 2 years ago

@AngeloGiurano Not expected, do you have a reproduction you can share? @ndelangen something you can help with?

ndelangen commented 2 years ago

@AngeloGiurano Please share the full logs / CLI output, just the

Could not find a declaration file for module 'history'.

Doesn't give me enough information to trace/fix the issue.

What I think might be happening is that WE removed our dependency on history package. So history isn't declaring any types anymore.

But somewhere you are importing history, this worked before because storybook added typings for history. Now it no longer is, and so you should install types for the history package yourself (if it's used somewhere).

But without a full log, this is speculation..