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

feat: use react hooks to manage styles injection #720

Merged
merged 3 commits into from
Aug 4, 2021
Merged

feat: use react hooks to manage styles injection #720

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
30d4d40
fix: safely remove style
huozhi Jul 9, 2021
551ba5d
remove props.children dep
huozhi Aug 3, 2021
48c7df8
BREAKING CHANGE: This change drops support for React version < 16.8.0
huozhi Aug 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@
"xo": "0.24.0"
},
"peerDependencies": {
"react": "15.x.x || 16.x.x || 17.x.x"
"react": ">= 16.8.0 || 17.x.x || 18.x.x"
},
"keywords": [
"babel-plugin-macros",
Expand Down
64 changes: 21 additions & 43 deletions src/style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,53 +1,31 @@
import { Component } from 'react'
import { useLayoutEffect } from 'react'
import StyleSheetRegistry from './stylesheet-registry'

const styleSheetRegistry = new StyleSheetRegistry()

export default class JSXStyle extends Component {
constructor(props) {
super(props)
this.prevProps = {}
}

static dynamic(info) {
return info
.map(tagInfo => {
const baseId = tagInfo[0]
const props = tagInfo[1]
return styleSheetRegistry.computeId(baseId, props)
})
.join(' ')
}

// probably faster than PureComponent (shallowEqual)
shouldComponentUpdate(otherProps) {
return (
this.props.id !== otherProps.id ||
// We do this check because `dynamic` is an array of strings or undefined.
// These are the computed values for dynamic styles.
String(this.props.dynamic) !== String(otherProps.dynamic)
)
}

componentWillUnmount() {
styleSheetRegistry.remove(this.props)
export default function JSXStyle(props) {
if (typeof window === 'undefined') {
styleSheetRegistry.add(props)
return null
Comment on lines +7 to +9
Copy link
Member

Choose a reason for hiding this comment

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

If we SSR the component, and somehow it's rendered multiple times (e.g. Suspense), will there be a problem?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The first time it is called add injects the styles into an in memory stylesheet. Future calls with the same props will just increment a counter

styled-jsx/src/stylesheet-registry.js

Lines 55 to 58 in 2741879

if (styleId in this._instancesCounts) {
this._instancesCounts[styleId] += 1
return
}

styled-jsx won't work with streaming rendering at the moment and perhaps suspense. I think it'd need a way to yield styles for the page while preserving a global state (registry) to avoid duplicates. I am open to collaborations if you'll ever need help with this.

Copy link
Member Author

@huozhi huozhi Jul 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Tested with suspense in react 18, looks good so far. As the anncoucement said effects hooks are reliable with suspense.
I use the streaming API with next.js to test it locally. But maybe the case is too simple.

@giuseppeg Would you mind make it more clear why you think it wouldn't work with streaming so far? Is it not safe to manage styles in memory even with hooks?

Copy link
Member

Choose a reason for hiding this comment

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

If we SSR the component, and somehow it's rendered multiple times (e.g. Suspense), will there be a problem?

To add more context to this, normally when we SSR all components will only be rendered once and there's no re-render. But with Suspense in the server side, it could be possible for a component to be re-rendered multiple times due to Suspense fallback and retry. In that case the counter will be wrongly counted. However since we won't have unmount in SSR and there's no style removal, so I think it's OK but I can't be 100% sure.

With CSR Suspense it won't be an issue though since we have every side effect inside the useLayoutEffect hook.

We will figure this out. 👍

Copy link
Collaborator

Choose a reason for hiding this comment

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

I use the streaming API with next.js to test it locally

@huozhi interesting, I'd be curious to see how this works but in general I was thinking that as you stream HTML you'd need to flush styles (render style tags) but keep an in memory registry on the server so that you don't accidentally render the same styles in future chunks (imagine you have 4 instances of a Button component).

Probably this could be hacked with the current utils but styled-jsx doesn't do this out of the box at the moment.

Copy link
Member

Choose a reason for hiding this comment

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

Note: streaming means multiple renders can happen concurrently, so a global registry on the server is unworkable with Concurrent Mode.

I wonder why is that unworkable. When doing SSR, the registry doesn't need to track the order of renders or concurrency, it just need to work like a boolean (only add the style when the counter turns from 0 to 1) since there's no removal process.

Or does it mean multiple requests, when you say "multiple renders" here?

Copy link

Choose a reason for hiding this comment

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

Yes, multiple requests can be being processed concurrently, so a singleton that tracks styles used wouldn’t work on the server, as other requests would read/write the same singleton.

Copy link

Choose a reason for hiding this comment

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

This works in synchronous React because the render starts and ends synchronously. But in React 18 a tree could suspend while waiting for data.

Copy link
Collaborator

@giuseppeg giuseppeg Jul 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is it possible for the following race condition to happen?

  1. big subtree starts rendering a Button which registers the styles
  2. smaller subtree renders Button and is ready before 1.
  3. the Button from 2. is unstyled

By the way a while ago I created a gist / PoC for a Style component that should work with streaming/suspense - I'll see if I can polish it and share it in the following days. As @devknoll mentioned it uses Context

Copy link
Member

Choose a reason for hiding this comment

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

Yeah... if that "smaller subtree renders Button and is ready before 1." is inside another Suspense boundary, it’s gonna be unstyled until it’s hydrated.

}

render() {
// This is a workaround to make the side effect async safe in the "render" phase.
// See https://github.com/zeit/styled-jsx/pull/484
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please read the comment below and see if you can verify that everything works fine i.e. triggering layout in a parent component doesn't run transitions. Before this was due to the fact that styles are appended after the DOM is updated.

@sebmarkbage explained what's going on here #484 (comment) This is by the way the reason why I was adding and removing styles during rendering - I needed them to be in the page before the related component was committed/updated

Copy link
Member Author

@huozhi huozhi Jul 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

useLayoutEffect is safer than useEffect here since useLayoutEffect inject the styles to DOM synchronously after DOM mutations which kind avoid the repaint happening very soon by browser.

We tried the jsfiddle example provided in that PR to test with react 17. If style injection happens in useEffect then it repaint but it doesn't repaint with useLayoutEffects.
https://codesandbox.io/s/react-17-repaint-demo-j21eq

Also found that emotion and styled-components both use useLayoutEffects to manage style injection. So I think this so be a fine solution so far? Before react releases any new hook for this case.

Copy link
Collaborator

@giuseppeg giuseppeg Jul 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Transitions are still running when you trigger layout https://codesandbox.io/s/react-17-repaint-demo-forked-c716t?file=/src/App.js

I'll leave it to you to decide whether this is ok but if I remember correctly I had to come up with that hack (inject while rendering) because of a bug report from Vercel folks.

Copy link
Member

Choose a reason for hiding this comment

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

As Sebastian said in that comment, it's missing a good solution and it feels OK for styled-jsx at the moment, as it's more like an uncovered case of React. We should either look forward to an official React API for this, or avoid immediate layout triggering in the application code.

I'll approve this PR since there will be a much bigger issue with the current implementation when enabling concurrent rendering in React 18.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good!

if (this.shouldComponentUpdate(this.prevProps)) {
// Updates
if (this.prevProps.id) {
styleSheetRegistry.remove(this.prevProps)
}

styleSheetRegistry.add(this.props)
this.prevProps = this.props
useLayoutEffect(() => {
styleSheetRegistry.add(props)
return () => {
styleSheetRegistry.remove(props)
}
// props.children can be string[], will be striped since id is identical
}, [props.id, String(props.dynamic)])
huozhi marked this conversation as resolved.
Show resolved Hide resolved
return null
}

return null
}
JSXStyle.dynamic = info => {
return info
.map(tagInfo => {
const baseId = tagInfo[0]
const props = tagInfo[1]
return styleSheetRegistry.computeId(baseId, props)
})
.join(' ')
}

export function flush() {
Expand Down