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

Turn off Emotion's warnings about potentially unsafe pseudo-selectors in renderDocs #18657

Closed
wants to merge 1 commit into from
Closed

Turn off Emotion's warnings about potentially unsafe pseudo-selectors in renderDocs #18657

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 9 additions & 3 deletions lib/preview-web/src/renderDocs.tsx
View file
Open in desktop
Expand Up @@ -2,10 +2,14 @@ import React, { ComponentType } from 'react';
import ReactDOM from 'react-dom';
import { AnyFramework } from '@storybook/csf';
import { Story } from '@storybook/store';
import { CacheProvider, createCache } from '@storybook/theming';

import { DocsContextProps } from './types';
import { NoDocs } from './NoDocs';

const emotionCache = createCache({ key: 'pr' });
emotionCache.compat = true;

export function renderDocs<TFramework extends AnyFramework>(
story: Story<TFramework>,
docsContext: DocsContextProps<TFramework>,
Expand Down Expand Up @@ -35,9 +39,11 @@ async function renderDocsAsync<TFramework extends AnyFramework>(
// Use `componentId` as a key so that we force a re-render every time
// we switch components
const docsElement = (
<DocsContainer key={story.componentId} context={docsContext}>
<Page />
</DocsContainer>
<CacheProvider value={emotionCache}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've decided to wrap this here, even though theme (and other things too) is provided in DocsContainer because I think that it's best not to bust the cache on this key change. In reality Emotion's cache can't be as easily busted and recreated (it's not impossible but I don't see why I would want to implement this here) so you'd end up with a lot of duplicated styles with each new key on the DocsContainer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually... perhaps this is not the best place to do this since DocsContainer is totally configurable and is being injected here:

storybook/addons/docs/src/preview.ts

Line 3 in 7ca3a53

getContainer: async () => (await import('./blocks')).DocsContainer,

So maybe I should actually finally look into fixing the real issue in Emotion 😅

Copy link
Member

Choose a reason for hiding this comment

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

@Andarist I'm not quite following:

perhaps this is not the best place to do this since DocsContainer is totally configurable and is being injected here

Why is that an issue? Are you thinking that maybe people might want a custom container that doesn't use this emotion cache business?

BTW on the subject of the key={} there -- it looks like it was added for the sake of angular (legacy) inline rendering: #16149

So perhaps we could drop it in 7.0 (as we are dropping the old inline rendering).

Copy link
Member

Choose a reason for hiding this comment

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

@Andarist having chatted with others, seeing as we are getting rid of the old inline rendering, the use of key={} here is no longer needed. With this in mind would you change the placement of the <CacheProvider>?

Copy link
Member

Choose a reason for hiding this comment

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

I'm happy to update the PR with both changes (dropping the key, and moving the provider) if you let me know!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's maybe wait a week or so before moving on with this PR - I will try to investigate this in the Emotion itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A little bit later than I've promised - but I took a look there and I think that I understand where Stylis puts the comments much better now. Gonna recheck a couple of things but I think that I'm close to fixing this.

Copy link
Member

Choose a reason for hiding this comment

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

< 2x the promised time is pretty darn good in my books!

<DocsContainer key={story.componentId} context={docsContext}>
<Page />
</DocsContainer>
</CacheProvider>
);

await new Promise<void>((resolve) => {
Expand Down