fix: change the way we initialize isBrowser #3122
Conversation
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit bf5e7d0:
|
|
with the above suggested solution the serializer could be flexible enough for the below to change from the storybook solution (or any other) code ref to the below This way the serializer will continue to work as is and also support emotion styled-components on storybook v7 |
|
Hey @Andarist is this something you could consider? 🤔 |
|
I need more context behind this. The requirement to prepare env before running some other files that depend on that context is a somewhat common practice. I also don't understand how JSDOM comes into play with Playwright here - shouldn't you be using a real browser with Playwright? |
|
The problem is that - storybook run each story written in .tsx files through playwright with their new test-runner and snapshot each case with jest. Historically having emotion serializer you could translate all your classnames into objects and easily see what changed from the style part. Since test-runner use playwright which doesn't give access to |
|
I don't think that this flexibility is quite needed here and it's a common practice to perform such checks once. An example of that can be found in React itself, see here. So while we could maybe change this here... I think that the overall problem would persist one way or another for Storybook users. That's why I'm looking for more context behind the Storybook requirements - so I could propose some alternative approach to solve this in the Storybook. @yannbf would you find some time to jump on a call with me to showcase the problem, an example setup of a project using this etc? |
Definitely! In fact I am meeting with the PR author in this link today at 1:10pm (about 40min from now) Poland time. I know it might be too soon, but otherwise I'm happy to jump on a call some other time later! |
|
Hey there @panvourtsis I met with @Andarist and we discussed about the problem space, this solution and next steps forward. Given the vast amount of use cases emotion is used in, it's not the best idea to introduce this change. It seems like this change would be added for a very special corner case, which could be circumvented in a different way. The way to achieve the same experience would be to utilize the import { JSDOM } from "jsdom";
import { TestRunnerConfig } from "@storybook/test-runner";
// this helper could come from a separate file
const createEmotionSerializer = () => {
const originalDocument = globalThis.document
// we make sure to patch document so that emotion's serializer doesn't throw
globalThis.document = originalDocument || {};
const { createSerializer } = require("@emotion/jest");
// we bring the original value back to not provide side effects elsewhere
globalThis.document = originalDocument;
return createSerializer();
}
const config: TestRunnerConfig = {
setup: () => {
expect.addSnapshotSerializer(createEmotionSerializer());
},
postRender: async (page, context) => {
const globalHTMLHandler = await page.$("html");
const globalHTML = await globalHTMLHandler.innerHTML();
const storyHTMLHandler = await page.$("#storybook-root");
const storyHTML = await storyHTMLHandler.innerHTML();
const { document: globalDocument } = new JSDOM(
`<!DOCTYPE html>${globalHTML}`
).window;
const { document: storyDocument } = new JSDOM(storyHTML).window;
globalThis.document = globalDocument; // ------> here document changed for playwright environments
expect(storyDocument.body).toMatchSnapshot();
},
};
export default config;However there are still other issues which we noticed. Emotion uses different algorithms to deal with styles in production and development. The above solution would only work when running tests against a development version of Storybook, and not when running the test-runner against a production build. For it to work in a production build, there would be quite some effort involved, by having to go through all the stylesheet rules and combine them, using a utility similar to this one. All in all I think, although the use case is valid, the changes in this PR are probably best not to be introduced. |
What:
Simple change of
isBrowserfrom constant to a function that takes an input and validates it on runtime.Why:
Due to the fact that the project
jest/serializeris checking the browser validity withisBrowserconstant on initialization of jest doesn't give the flexibility to consumers to prepare thatdocument. Therefore if we simply change the use to a function an passdocumentas a property it can verify it on runtime rather than initialization.This is a known issue to all of the storybook community now with the new
test-runnerfunctionality which adds playwright under the hoodIssues:
Storybook issue #1
Storybook issue #2
How:
N/A
Checklist:
I haven't touched the below items as the change doesn't affect any