storybookjs / storybook

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

[Bug]: next/router causing issues #21654

Open arianaamx-motorway opened 1 year ago

arianaamx-motorway commented 1 year ago

Describe the bug

Stack:

next: v12.1
storybook: v7.0.0-rc.3

There is a Button with logic that breaks the Story:

import Router from 'next/router';

<Button disabled={!isSSR && Router.pathname === '/signup'}/>

The part that breaks is the Router.pathname . Error message:

No router instance found.
You should only use "next/router" on the client side of your app.

I tried experimenting with nextjs object inside story, like so, but it didn't help:

  nextjs: {
    router: {
      basePath: '/',
      route: '/signup',
    },
  },

Any help with this?

To Reproduce

arianaamx-motorway/next-router-no-router-instance-found

Stackblitz DEMO: https://stackblitz.com/github/arianaamx-motorway/next-router-no-router-instance-found?file=stories/pages/HomePage/HomePage.tsx

System

Environment Info:

  System:
    OS: macOS 12.5.1
    CPU: (8) arm64 Apple M1
  Binaries:
    Node: 16.16.0 - ~/.nvm/versions/node/v16.16.0/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v16.14.2/bin/yarn
    npm: 8.11.0 - ~/.nvm/versions/node/v16.16.0/bin/npm
  Browsers:
    Chrome: 111.0.5563.64
    Firefox: 111.0
    Safari: 15.6.1
  npmPackages:
    @storybook/addon-essentials: ^7.0.0-rc.3 => 7.0.0-rc.3 
    @storybook/addon-interactions: ^7.0.0-rc.3 => 7.0.0-rc.3 
    @storybook/addon-links: ^7.0.0-rc.3 => 7.0.0-rc.3 
    @storybook/blocks: ^7.0.0-rc.3 => 7.0.0-rc.3 
    @storybook/nextjs: ^7.0.0-rc.3 => 7.0.0-rc.3 
    @storybook/react: ^7.0.0-rc.3 => 7.0.0-rc.3 
    @storybook/testing-library: ^0.0.14-next.1 => 0.0.14-next.1 

Additional context

No response

arianaamx-motorway commented 1 year ago

Hi @shilman,

I have added the repo with a reproducible bug: arianaamx-motorway/next-router-no-router-instance-found

And Stackblitz DEMO: https://stackblitz.com/github/arianaamx-motorway/next-router-no-router-instance-found?file=stories/pages/HomePage/HomePage.tsx

Hopefully, this helps with solving the problem!

shilman commented 1 year ago

Thank you so much @arianaamx-motorway !! We'll try to take care of it soon 🙏

valentinpalkovic commented 1 year ago

Hi @arianaamx-motorway,

I have investigated your Stackblitz demo (thanks for that), and I see a couple of issues:

First of all, I saw it the first time, that Router as a singleton is imported to use properties like "pathname". I am pretty sure, that this is not the recommended way, and you should use useRouter instead, like described here: https://nextjs.org/docs/api-reference/next/router.

Second, the properties in the stories were not defined properly to set pathname to /signup. I have created a corrected Stackblitz example: https://stackblitz.com/edit/github-vcrdnv

arianaamx-motorway commented 1 year ago

Hi @valentinpalkovic,

We have Class components, and by default can't use useRouter hook. Router as singleton is still supported and we had no issues with it ever.

While we were using Storybook 7.0.0-beta.52 we mocked the router with storybook-addon-next-router. I am trying to update it to the latest released version, and trying to mock it with the addon or without it doesn't work anymore.

I corrected the properties in the stories, still no luck.

valentinpalkovic commented 1 year ago

@arianaamx-motorway Can you point me to the Next.js documentation, where this singleton pattern is described?

arianaamx-motorway commented 1 year ago

Well, I can't find it. They are recommending useRouter and withRouter.

But, there is a good discussion about the difference between the two, and when you should use singleton vs hook: https://github.com/vercel/next.js/discussions/18522#discussioncomment-2417009

As I said, it worked until I tried to update to the latest RC release.

valentinpalkovic commented 1 year ago

The comment you are referring to is 1 year old. If the API is not documented officially, I would recommend to not use it, because it can change without warnings. So without having a official documentation available, I don't think that we will support this use case in the near future. But feel free to create a PR to add this functionality.

valentinpalkovic commented 1 year ago

Let me know when the singleton import is an officially documented API. We will then take another look at it. For now, I marked this issue as won't fix and close the issue.

bordeo commented 1 year ago

I've successfully mocked the singletonRouter in this way. Add new file .stoybook/mock-router.js

.storybook/mock-router.js

```javascript import singletonRouter from 'next/router' import { action } from '@storybook/addon-actions' export const createMockRouter = (...args) => { singletonRouter.router = newMockRouter(...args) singletonRouter.readyCallbacks.forEach((cb) => cb()) singletonRouter.readyCallbacks = [] return singletonRouter.router } export const defaultRouterParams = { pathname: '/', query: {}, } /** * Copied from: * https://github.com/storybookjs/storybook/blob/ca3901cd39a54b94caf0c9ab68678803670874aa/code/frameworks/nextjs/src/routing/page-router-provider.tsx#L19 */ export const newMockRouter = ({ globals, routeParams }) => ({ push(...args) { action('nextRouter.push')(...args) return Promise.resolve(true) }, replace(...args) { action('nextRouter.replace')(...args) return Promise.resolve(true) }, reload(...args) { action('nextRouter.reload')(...args) }, back(...args) { action('nextRouter.back')(...args) }, forward() { action('nextRouter.forward')() }, prefetch(...args) { action('nextRouter.prefetch')(...args) return Promise.resolve() }, beforePopState(...args) { action('nextRouter.beforePopState')(...args) }, events: { on(...args) { action('nextRouter.events.on')(...args) }, off(...args) { action('nextRouter.events.off')(...args) }, emit(...args) { action('nextRouter.events.emit')(...args) }, }, locale: globals?.locale, route: '/', asPath: '/', basePath: '/', isFallback: false, isLocaleDomain: false, isReady: true, isPreview: false, ...routeParams, }) ```

In your .storybook/preview.js, please initialize the singletonRouter.

import { createMockRouter, defaultRouterParams } from './mock-router'

const preview = {
    loaders: [
        async ({ globals, parameters }) => ({
            router: createMockRouter({
                globals,
                routeParams: {
                    ...defaultRouterParams,
                    ...parameters.nextjs?.router,
                },
            }),
        }),
    ],
}

@valentinpalkovic, if you agree with my code, I can create a pull request that uses the provider default values and manages both AppRouter and PageRouter.

oosawy commented 7 months ago

As background information, this export is marked as "Export the singletonRouter and this is the public API". Hope this API will be supported as it is well known by some users and there are projects where it is widely used

https://github.com/vercel/next.js/blob/9638b9d46456a98b48fba3fdb01e4ef4cc197b56/packages/next/src/client/router.ts#L126-L127