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.

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

Conversation

huozhi
Copy link
Member

@huozhi huozhi commented Jul 9, 2021

StyleSheetRegistry is an external state outside of react, to keep sync with react internal state, we'd better use reliable lifecycle to count the change instead of use cached class properties and render function to determine changes.

Lifecycle

first render -> add style
update -> remove then add
unmount -> remove

Problem

the issue is the react internal state might be different with the cached class properties this.prevProps, react might drop some render due to prioritise change, leading to tearing.

Notice

This PR only solved the effects issue when using the react under concurrent mode.

@huozhi huozhi requested a review from giuseppeg as a code owner July 9, 2021 08:38
@huozhi huozhi force-pushed the fix/deleted-style-id branch from 38bc460 to 9ab4a77 Compare July 9, 2021 08:41
@shuding
Copy link
Member

shuding commented Jul 9, 2021

Is this caused by strict effects or is it something else? Could you link more references in the description? Would appreciate it!

componentWillUnmount is keeping called from react flushSyncCallbacks in react 18

@huozhi huozhi marked this pull request as draft July 9, 2021 09:19
@huozhi huozhi marked this pull request as ready for review July 9, 2021 14:19
package.json Outdated Show resolved Hide resolved
@huozhi huozhi force-pushed the fix/deleted-style-id branch from 8a93b81 to 1ded933 Compare July 9, 2021 14:29
@Timer
Copy link
Member

Timer commented Jul 9, 2021

If you end up dropping an old react version, make sure you add the following as a new line to the commit message when squash & merged:

BREAKING CHANGE: This change drops support for React 15.

Adding that line to the commit message will ensure this releases as a new major!

@huozhi huozhi marked this pull request as draft July 13, 2021 14:46
@huozhi huozhi force-pushed the fix/deleted-style-id branch from 629c0b9 to e47f2c1 Compare July 13, 2021 15:10
@huozhi huozhi marked this pull request as ready for review July 13, 2021 15:26

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

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

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!

src/style.js Show resolved Hide resolved
Comment on lines +7 to +9
if (typeof window === 'undefined') {
styleSheetRegistry.add(props)
return null
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

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

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?

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.

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

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.

@huozhi huozhi force-pushed the fix/deleted-style-id branch from 74c1e9a to e31dbb6 Compare July 22, 2021 11:13
BREAKING CHANGE: This change drops support for React 15 and 16
@huozhi huozhi force-pushed the fix/deleted-style-id branch from 11aa92b to 30d4d40 Compare August 3, 2021 08:44
package.json Outdated Show resolved Hide resolved
giuseppeg
giuseppeg previously approved these changes Aug 4, 2021
Copy link
Collaborator

@giuseppeg giuseppeg left a comment

Choose a reason for hiding this comment

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

Check the peerDependencies comment and then feel free to merge. Remember to add the comment below when squashing

BREAKING CHANGE: This change drops support for React 15.

@giuseppeg
Copy link
Collaborator

@huozhi I am thinking that since there will be more breaking changes in the other PRs maybe we can create a canary release and once we merge all the changes necessary to support React18 we can cut a major release

@huozhi
Copy link
Member Author

huozhi commented Aug 4, 2021

@giuseppeg yeah totally, @shuding and I were thinking the same thing. I'll update the alpha branch to support prereleases. 🙏

@huozhi huozhi changed the base branch from master to next August 4, 2021 12:07
@huozhi huozhi changed the title fix: safely remove style feat: use react hooks to manage styles injection Aug 4, 2021
@huozhi huozhi deleted the branch vercel:alpha August 4, 2021 12:13
@huozhi huozhi closed this Aug 4, 2021
@huozhi huozhi reopened this Aug 4, 2021
@huozhi huozhi changed the base branch from next to alpha August 4, 2021 12:19
@huozhi huozhi force-pushed the alpha branch 2 times, most recently from 0b28b97 to 20396d2 Compare August 4, 2021 13:12
@huozhi huozhi merged commit bdc8d5d into vercel:alpha Aug 4, 2021
@huozhi huozhi deleted the fix/deleted-style-id branch August 4, 2021 13:19
@github-actions
Copy link

github-actions bot commented Aug 4, 2021

🎉 This PR is included in version 4.0.0-alpha.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Timer pushed a commit that referenced this pull request Aug 9, 2021

Unverified

This user has not yet uploaded their public signing key.
* fix: safely remove style

* remove props.children dep

* BREAKING CHANGE: This change drops support for React version < 16.8.0
huozhi added a commit that referenced this pull request Aug 9, 2021
* fix: safely remove style

* remove props.children dep

* BREAKING CHANGE: This change drops support for React version < 16.8.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants