Turn off Emotion's warnings about potentially unsafe pseudo-selectors in renderDocs
#18657
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit ccf55cc. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
| <DocsContainer key={story.componentId} context={docsContext}> | ||
| <Page /> | ||
| </DocsContainer> | ||
| <CacheProvider value={emotionCache}> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
So maybe I should actually finally look into fixing the real issue in Emotion 😅
There was a problem hiding this comment.
@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).
There was a problem hiding this comment.
@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>?
There was a problem hiding this comment.
I'm happy to update the PR with both changes (dropping the key, and moving the provider) if you let me know!
There was a problem hiding this comment.
Let's maybe wait a week or so before moving on with this PR - I will try to investigate this in the Emotion itself.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
< 2x the promised time is pretty darn good in my books!
| <DocsContainer key={story.componentId} context={docsContext}> | ||
| <Page /> | ||
| </DocsContainer> | ||
| <CacheProvider value={emotionCache}> |
There was a problem hiding this comment.
@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).
|
Hey @tmeasday This was assigned to me to bring it up to date with |
|
Has been fixed in another PR, AFICS |
Similar to #18361
fixes: #18656
Please retest this before merging.