Storybook global mock useDetectOS and NodeReleasesProvider

[HinataKah0]

Enter your suggestions in details:

NodeReleasesProvider is used in Storybook now. However, it is reading from generated public/node-releases-data.json file now.
https://github.com/nodejs/nodejs.org/blob/main/next.app.tsx#L30-L34

And similarly with useDetectOS hook, it will detect your PC/laptop's OS and use it for generating the Storybook snapshots.
See this generated snapshot.

Notice that the Mac's DownloadCard is having downloadCardActive class:

<li class="DownloadCard_downloadCard__eWZZG DownloadCard_downloadCardActive__mwG_u"
    role="tab"
    id="tabdownload-card-MAC"
    aria-selected="false"
    aria-disabled="false"
    aria-controls="paneldownload-card-MAC"
    tabindex="0"
    data-rttab="true"
>

I think we need a global mock since many components depend on Node releases data and user OS information.

[HinataKah0]

For NodeReleasesProvider:

I think we can add a prop in BaseApp to indicate if it's being rendered in Storybook or not. If yes, we'll supply the data from the fixture.

[ovflowd]

I think we can add a prop in BaseApp to indicate if it's being rendered in Storybook or not. If yes, we'll supply the data from the fixture.

I don't think we need to do that, I guess we can simply override the JSON import with Jest Module Mocks.

[ovflowd]

Our App should not have test-specific logic. The testing environments should be able to mock/spy on the real stuff.

[HinataKah0]

I am considering jest.mock as well...
And I don't think we can only mock the JSON file import because status depends on new Date() as well.

I think we can mock the hooks here which only affects the snapshot tests. They can be added to setup().

Will give this a try in my PR...

[ovflowd]

Yeah, in the end we just want to mock the usage of the data within Storybook, so even if the Provider has the real deal, it's fine if the hooks provide the mocked data.

[HinataKah0]

It seems like I can't use jest.mock() (tried it myself as well), similar to this issue

I guess I'll try to find a workaround or maybe figuring out why it doesn't work in the first place...

[ovflowd]

I guess I'll try to find a workaround or maybe figuring out why it doesn't work in the first place...

What doesn't work in the first place?

We can also have a fake App Tree within the preview.tsx, we don't need to call the real App Tree from next.app.mjs, we can ignore all the Intl part, Data part all these things.

The App Tree for Storybook should be as simple as possible. I would even argue that the Theming we have (switch from dark to light) should not be done via ThemeProvider but by Storybook's Theme Toggle that can inject the same classname that ThemeProvider does...

[HinataKah0]

Sorry for the confusion

What doesn't work in the first place?

Initially I wanted to mock the modules for only snapshots testing (and having Storybook running using real data).
However, we can't globally mock the data in setup() because Storybook and test-runner is running at the same time. They're not running within the same process thus jest.mock won't work as I described previously.

I just remembered Harkunwar's moduleMock.
I agree the Storybook tree shouldn't be as complicated as the real tree. Maybe I'll just mock in the Story itself (similar to Harkunwar's) rather than trying the global mock in this case, since it gives flexibility for each Story to mock some edge cases if necessary (similar to tests).

[ovflowd]

Yeah, to be fair, I still would argue we shouldn't import the whole Base App tree on Storybooks (https://github.com/nodejs/nodejs.org/blob/main/.storybook/preview.tsx#L30), the idea initially was of course to the Stories to be as close as the real, App.

But thinking it through we have a couple of issues:

• The BaseApp contains the ThemeProvider and MotionConfig but these aren't used on the current legacy website (https://github.com/nodejs/nodejs.org/blob/main/next.app.tsx#L28-L29)
• We don't need the ReleaseProvider and BlogDataProvider on Storybook. It's okay if it shows the message ID's, IMHO.

I'm going to make a quick-pr making some changes that should ease your work.

[ovflowd]

@HinataKah0 let me know if you need help here :)

[ovflowd]

FYI @HinataKah0 do you have any progress here? I assume the useDetectOS is still needed 👀

[ovflowd]

Note that based on the new Figma's our useDetectOS might now need to provide more options:

• If you're on Linux
• If your architecture is x64 or x86 or ARM

[HinataKah0]

I tried mocking the useDetectOS hook for Storybook's snapshot test (credit to Harkunwar) here.

And mocking for unit tests here (in the same PR).

I've been putting the PR on hold due to the new design. And the issue has been closed as well.

Since some time has passed, I want to validate my understanding again (it's very easy to miscommunicate). From what I understood, I think we previously agreed on mocking the useDetectOS in each individual unit test & snapshot test.

[AugustinMauroy]

Do we need this anymore?