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

feat: contextual styled-jsx #721

Merged
merged 12 commits into from
Sep 3, 2021
Merged

feat: contextual styled-jsx #721

merged 12 commits into from
Sep 3, 2021

Conversation

huozhi
Copy link
Member

@huozhi huozhi commented Jul 22, 2021 •
edited
Loading

Feature

This is the draft for contextual styled-jsx rendering idea from https://gist.github.com/giuseppeg/dc4cecdae1e9fcb4c5761712e75e9b93

Which would be a benefit for multi-requests case with concurrent mode on server side, metioned in #720 .
Each request will only use its own context to add/remove styles, to avoid having a shared style registry instance messed up on server side.

Use case

import React from 'react'
import ReactDOM from 'react-dom/server'
import { StyleRegistry, useStyleRegistry } from 'styled-jsx'
import App from './app'

function Head() {
  const registry = useStyleRegistry()
  const styles = registry.styles()
  // in blocking rendering, flush styles in each render
  registry.flush()
  return <head>{styles}</head>
}

export default (req, res) => {
  const app = ReactDOM.renderToString(<App />)
  const html = ReactDOM.renderToStaticMarkup(
    <StyleRegistry>
      <html>
        <Head />
        <body>
          <div id="root" dangerouslySetInnerHTML={{ __html: app }} />
        </body>
      </html>
    </StyleRegistry>
  )
  res.end('<!doctype html>' + html)
}

Closes #703
Closes #485
Closes #563

@huozhi
Copy link
Member Author

huozhi commented Jul 22, 2021

Draft the idea for moving style registry to react context for safety in concurrent mode. The APIs are not stabilized, feel free to share your opinions about it.
@giuseppeg @shuding @devknoll

@huozhi huozhi force-pushed the contextual-styles branch from 9a4118e to 969e505 Compare July 22, 2021 16:19
giuseppeg
giuseppeg reviewed Jul 27, 2021
View reviewed changes
Copy link
Collaborator

@giuseppeg giuseppeg left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for putting this together @huozhi

I am afraid that the moment we introduce a configurable registry via context we open up the possibility to get into this issue #703 (comment) i.e. if you have multiple registries in the page the first one will pick up all the server side rendered style tags regardless of where they came from (which registry they belong to).

In general if you want styled-jsx to work with modern React you'll have to change quite a few things and it would be worth considering all the constraints and edge cases together (concurrent mode, streaming rendering, multiple registries/context etc).

Unfortunately I don't have time to do all this work on my spare time but if you all put together a detailed proposal/rfc I'd be happy to take a look.

fwiw the gist you found has flaws but I haven't had the time to iterate on it - I just made it public because I thought nobody would find it :)

test/babel6/stylesheet-registry.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
src/server.js Outdated Show resolved Hide resolved
src/style.js Outdated Show resolved Hide resolved
@huozhi huozhi changed the title Contextual styles feature: contextual styled-jsx Jul 28, 2021
@huozhi huozhi force-pushed the contextual-styles branch from 9568893 to 9a2742e Compare August 10, 2021 19:28
giuseppeg
giuseppeg reviewed Aug 11, 2021
View reviewed changes
src/style.js Outdated Show resolved Hide resolved
@huozhi huozhi force-pushed the contextual-styles branch from 9a2742e to 443d6f0 Compare August 17, 2021 16:30
@huozhi huozhi changed the title feature: contextual styled-jsx feat: contextual styled-jsx Aug 17, 2021
@huozhi huozhi force-pushed the contextual-styles branch from 5969020 to 1a4e35e Compare August 27, 2021 18:39
@huozhi huozhi changed the base branch from master to alpha August 27, 2021 18:40
@huozhi huozhi marked this pull request as ready for review August 27, 2021 18:40
@huozhi huozhi deleted the branch vercel:alpha August 31, 2021 07:42
@huozhi huozhi closed this Aug 31, 2021
@huozhi huozhi reopened this Aug 31, 2021
@huozhi huozhi force-pushed the contextual-styles branch from 8df01c5 to eaa5461 Compare August 31, 2021 08:02
@huozhi huozhi marked this pull request as draft August 31, 2021 15:44
@huozhi huozhi requested a review from shuding September 1, 2021 08:01
shuding
shuding reviewed Sep 1, 2021
View reviewed changes
src/stylesheet-registry.js Outdated Show resolved Hide resolved
src/stylesheet-registry.js Outdated Show resolved Hide resolved
src/stylesheet-registry.js Show resolved Hide resolved
@huozhi huozhi mentioned this pull request Sep 2, 2021
Merged
2 tasks
@huozhi huozhi marked this pull request as ready for review September 2, 2021 15:12
shuding
shuding approved these changes Sep 2, 2021
View reviewed changes
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@huozhi
Copy link
Member Author

huozhi commented Sep 3, 2021

going to merge this PR into alpha branch to create a prerelease now. any further concern with this PR please leave comments I'll address them later 🙏 cc @giuseppeg

@huozhi huozhi merged commit c2cbe52 into vercel:alpha Sep 3, 2021
@huozhi huozhi deleted the contextual-styles branch September 3, 2021 15:31
@huozhi huozhi restored the contextual-styles branch September 3, 2021 19:47
@huozhi huozhi deleted the contextual-styles branch September 3, 2021 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devknoll devknoll devknoll left review comments

@giuseppeg giuseppeg giuseppeg left review comments

@shuding shuding shuding approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@huozhi @giuseppeg @devknoll @shuding