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

HOC should not attach sheets until mount #1149

Merged
merged 3 commits into from
Jul 6, 2019
Merged

Conversation

felthy
Copy link
Member

@felthy felthy commented Jul 3, 2019

Fixes #1143.

Per the discussion in #1140 and, prior to that, #1089, sheets should not be attached during server-side rendering.

Now that #1148 has been merged, server rendering is working for the hook API (createUseStyles()) but the HOC (withStyles()) hasn't been updated yet so still warns (incorrectly) Rule is not linked. Missing sheet option "link: true" on the server.

This PR brings the HOC implementation into line with the hook implementation - adding the sheet to the SheetsRegistry when the component is constructed, and not attaching (managing) it until the component is mounted (which never occurs on the server).

@felthy felthy requested a review from HenriBeck as a code owner July 3, 2019 01:36
@felthy felthy requested a review from kof July 3, 2019 01:36
@felthy felthy requested a review from HenriBeck July 3, 2019 22:59
@HenriBeck HenriBeck merged commit 675d934 into master Jul 6, 2019
@kof
Copy link
Member

kof commented Jul 7, 2019

Changelog forgotten!

@kof
Copy link
Member

kof commented Jul 7, 2019

Looking at this, we shouldn't be calling .attach in constructor at all, this should be done in componentDidMount

@HenriBeck
Copy link
Member

Wouldn’t we then have a flash of uncomputed styles?

@kof
Copy link
Member

kof commented Jul 7, 2019

Nope, didMount happens synchronously after rendering, so one can still do stuff like that, before browser starts drawing

@kof
Copy link
Member

kof commented Jul 7, 2019

I had a discussion with the core team about this a while ago, it sounds dangerous but it actually is guaranteed.

HenriBeck pushed a commit that referenced this pull request Jul 7, 2019
* master:
  Use componentDidMount for WithStyles (#1157)
  Use mui like global ponyfill (#1153)
  HOC should not attach sheets until mount (#1149)
  v10.0.0-alpha.22
  add react-dom dev dependenncy
  Fix SSR for Hooks based implementation (#1148)
  React-JSS id prop docs and improvements (#1147)
  v10.0.0-alpha.21
  fix changelog
  add support hint to the changelog (#1145)
  Sheets management for css() (#1137)

# Conflicts:
#	packages/react-jss/.size-snapshot.json
bhupinderbola pushed a commit to bhupinderbola/jss that referenced this pull request Sep 17, 2019
* HOC should not attach sheets until mount, same as the new hook implementation

* Fix potential FOUC by attaching sheet in HOC constructor when running in browser.

* Call registry.add() on the client too for parity with the hooks implementation
@BenjaminWFox-Lumedic
Copy link

Unsure if this is the right place to ask this but I noticed a change in test behavior (jest+enzyme, react-jss components) which i'm wondering is a side-effect of this update?

It's not a problem, I'm just curious if I'm understanding the functionality I'm seeing & if there are any other changes I missed that might contribute (I just updated react-jss from around ~8.6.* to 10.0.0).

Previously components wrapped in withStyles and shallow rendered with enzyme would give an error when no ThemeProvider was present.

After updating the same components shallow rendered don't give any errors. Using the enzyme method mount does still give an error.

I think this makes sense, because enzyme it isn't fully mounting the component with shallow, so there is no need for react-jss to find the ThemeProvider to actually attach the necessary sheets?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect "Rule is not linked" warning on server when using function rules
4 participants