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.

Sign up for GitHub

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

Jump to bottom

Addon-interactions: Mock window in browser environments #17535

Merged
merged 3 commits into from Feb 25, 2022
Merged

Addon-interactions: Mock window in browser environments #17535

merged 3 commits into from Feb 25, 2022

Conversation

IanVS
Copy link
Member

@IanVS IanVS commented Feb 18, 2022

Issue: #17516

What I did

Importing fn directly from jest-mock does not work in a browser, since jest expects to run in node and mocks out global instead of window (https://github.com/facebook/jest/blob/465518a2fa17ce45f7e3280dd0f54351273432c4/packages/jest-mock/src/index.ts#L1200-L1202). This breaks in vite, which does not polyfill global like webpack does.

The change here was to use the global package, as is used elsewhere in storybook, to determine which global to mock (window in browsers, global in node).

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

I've tested this by building, copying dist into my project, and verifying that the interactions addon works correctly, whereas it crashes if I just install the addon from npm.

@nx-cloud
Copy link

nx-cloud bot commented Feb 18, 2022 •
edited

☁️ Nx Cloud Report

CI ran the following commands for commit 5952b57. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@IanVS IanVS changed the title Mock window in browser environments [addon-interactions] Mock window in browser environments Feb 18, 2022
@IanVS IanVS mentioned this pull request Feb 18, 2022
Closed
@IanVS
Copy link
Member Author

IanVS commented Feb 22, 2022 •
edited

I spent a few hours trying to get E2E tests running locally, and I have to give up now. :(.

I'm running yarn local-registry --port 6000 --open, but no matter what I try, I always get errors like

@storybook/addon-actions@npm:6.5.0-alpha.40: The remote server failed to provide the requested resource
➤ YN0035: │   Response Code: 404 (Not Found)
➤ YN0035: │   Request Method: GET
➤ YN0035: │   Request URL: http://localhost:6000/@storybook%2faddon-actions

during the 🎨 Initializing Storybook with @storybook/cli phase.

I can copy/paste the dist folder into an example, and check it there manually for now.

@IanVS
Copy link
Member Author

IanVS commented Feb 22, 2022

e2e test is now passing. :success:

@IanVS IanVS requested a review from yannbf February 22, 2022 04:20
@IanVS
Copy link
Member Author

IanVS commented Feb 22, 2022

Hah, looks like jest took care of this in their latest alpha too. jestjs/jest#12447

yannbf
yannbf approved these changes Feb 25, 2022
View reviewed changes
Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

LGTM! Great stuff @IanVS

@yannbf yannbf changed the title [addon-interactions] Mock window in browser environments Addon-interactions: Mock window in browser environments Feb 25, 2022
@yannbf yannbf merged commit 9749c0d into next Feb 25, 2022
@yannbf yannbf deleted the jest-mock branch February 25, 2022 16:48
@shilman shilman mentioned this pull request Feb 28, 2022
Closed
IanVS
IanVS commented Mar 2, 2022
View reviewed changes
addons/interactions/src/preset/argsEnhancers.ts
import { FORCE_REMOUNT, STORY_RENDER_PHASE_CHANGED } from '@storybook/core-events';
import { AnyFramework, ArgsEnhancer } from '@storybook/csf';
import { instrument } from '@storybook/instrumenter';
import { ModuleMocker } from 'jest-mock';

const { fn } = new ModuleMocker(globalWindow);
const JestMock = new ModuleMocker(global);
Copy link
Member Author

Choose a reason for hiding this comment

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

Ugh, I screwed this up, it needed to be globalThis. I'll need to send another PR to fix this.

@IanVS IanVS mentioned this pull request Mar 2, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yannbf yannbf yannbf approved these changes

Assignees
No one assigned
Labels
addon: interactions bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@IanVS @yannbf