Use React hoistable styles #3295
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. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
|
Andarist
force-pushed
the
use-hoistable-styles
branch
from
c12a777 to
19a5585
Compare
| <style | ||
| data-href="1lrxbo5" | ||
| data-precedence="emotion-test" | ||
| > | ||
| .test-1lrxbo5{color:hotpink;} | ||
| </style> | ||
| <style | ||
| data-emotion="test-global" | ||
| data-s="" | ||
| > | ||
|
|
||
| /** test-global */body{width:0;} | ||
| </style> |
There was a problem hiding this comment.
their order flipped, we can fix this by using precedence with -global on global styles, that will allow people to control how they are grouped - they could render dummy styles in head with those precedence set and then React will insert new styles there
the problem is that with those hoistable styles... we lose the ability to unmount global styles, are we fine with that?
There was a problem hiding this comment.
we lose the ability to unmount global styles, are we fine with that?
imo, yes, it's fine. I've never been a big fan of the unmounting styles is Global, it's kind of started out as "i guess we can do this so why not" but imo it's not really that valuable since people should just scope styles (whether that's literally with the css prop/etc. or with global styles but all the styles are scoped to some selector) and conditionally apply the selector rather than have conditional style insertion.
There was a problem hiding this comment.
With this change - if we use hoistable styles everywhere - we won't need any SSR APIs at all (🎉). We can keep them for vanilla Emotion but then we'll need to tweak how our packages compose and how they are organized (like we'll have to move hydration paths to vanilla Emotion code parts instead of keeping them in @emotion/cache).
So at the moment, I didn't bother to tweak this. We need to first establish the baselines functionality~ for those hoistable styles
| nonce={cache.sheet.nonce} | ||
| href={current.name} | ||
| > | ||
| {maybeStyles ?? styles} |
There was a problem hiding this comment.
this is nasty, when finalizing this PR we'll have to rethink the ssr/browser paths used by cache.insert. In the case of React we always want to get them back as the return value (so again, maybe we should rethink how our packages are composed)
| output.push( | ||
| <style | ||
| precedence={`emotion-${cache.key}`} | ||
| nonce={cache.sheet.nonce} | ||
| href={current.name} | ||
| /> | ||
| ) |
There was a problem hiding this comment.
from what I understand we don't have to return the styles for already inserted styles... so I just return empty style tags here to keep the render returns somewhat idempotent (and that might have some impact on rehydration too, especially for useId as React is using sibling/descendant/whatever counters to rehydrate those correctly)
There was a problem hiding this comment.
we might have to dust off our benchmarks... React doesn't use CSSOM APIs at all, we need to check how fine we are with that
Andarist
force-pushed
the
use-hoistable-styles
branch
from
f505a06 to
09bcb47
Compare
Andarist
force-pushed
the
use-hoistable-styles
branch
from
09bcb47 to
0439d5a
Compare
No description provided.