cssinjs / jss

JSS is an authoring tool for CSS which uses JavaScript as a host language.
https://cssinjs.org
MIT License
7.07k stars 396 forks source link

"SheetsManager: can't find sheet to unmanage" on react-jss@10-alpha #981

Closed iamstarkov closed 5 years ago

iamstarkov commented 5 years ago

I can consistently reproduce the error but can't think of minimum reproducible demo as of now.

For background: I use react-loadable and react-router and theming from react-jss, webpack 4 and babel 7. Warning is being triggered when I navigate from one code-splitted route to another one. All page related components are being unmounted thus sheets are to be unmanaged, but apparently it is not there to unmanage. Out of 50+ components for the sheet to be unmounted only few (4-7) are triggering the warning. Perhaps not all of the components are themed.

The only relevant issue or PR I could find is #834 and I looked into it and can not tell that regression has been introduced in that PR. If I rollback to react-jss@8.6.1 the error is no longer there.

If no one else has seen this error or the root cause is not obvious I can try to construct a minimum reproducible demo. The reason for not doing so from the start is early feedback for the alpha release and late evening, I'm sorry.

kof commented 5 years ago

Try removing depenencies one after another in the project you are until you have a minimum that you can mimic on codesandbox?

iamstarkov commented 5 years ago

It is happening when you have a theming in place but component isn't themed

kof commented 5 years ago

Still not clear)

iamstarkov commented 5 years ago

kapture 2019-01-15 at 11 42 11

iamstarkov commented 5 years ago
import React from 'react';
import { BrowserRouter as Router, Route, Link, Switch, Redirect } from "react-router-dom";
import injectSheet, {ThemeProvider} from 'react-jss'

const theme = {};

const A = injectSheet({ rootA: {} })(props => (<div className={props.classes.rootA} />));
const B = injectSheet({ rootB: { color: p => !!p.whatever ? 'white' : 'black' } })(props => (<div className={props.classes.rootB} />));
const C = injectSheet(theme => ({ rootC: {} }))(props => (<div className={props.classes.rootC} />));
const D = injectSheet(theme => ({ rootD: { color: p => !!p.whatever ? 'white' : 'black' }}))(props => (<div className={props.classes.rootD} />));

const AppRouter = () => (
  <Router>
    <ThemeProvider theme={theme}>
      <div>
        <nav>
          <Link to="/a">Non-Themed, Non-Dynamic (WARNING)</Link>
          <br />
          <Link to="/b">Non-Themed, Dynamic (WARNING)</Link>
          <br />
          <Link to="/c">Themed, Non-Dynamic</Link>
          <br />
          <Link to="/d">Themed, Dynamic</Link>
        </nav>
        <Switch>
          <Route path="/a" exact component={A} />
          <Route path="/b" exact component={B} />
          <Route path="/c" exact component={C} />
          <Route path="/d" exact component={D} />
          <Redirect from="/" to="/a" />
        </Switch>
      </div>
    </ThemeProvider>
  </Router>
);

export default AppRouter;
kof commented 5 years ago

What you mean (I assume) is that the tree is wrapped with a ThemeProvider but a component styles is an object?

iamstarkov commented 5 years ago

here is a demo https://github.com/iamstarkov/jss-981

git clone git@github.com:iamstarkov/jss-981.git
cd jss-981
yarn
yarn start
iamstarkov commented 5 years ago

What you mean (I assume) is that the tree is wrapped with a ThemeProvider but a component styles is an object?

yes. Certain components dont necessarily need to rely on theme to be styled

kof commented 5 years ago

cool, thanks, lets fix this one before releasing the next alpha

kof commented 5 years ago

There is a big refactoring here https://github.com/cssinjs/jss/pull/949 it might have solved it by accident btw.

kof commented 5 years ago

Also we should have a test for this scenario

HenriBeck commented 5 years ago

Let's maybe release the next alpha with the big refactor and see if it's still an issue

kof commented 5 years ago

yeah, we can do it too, @iamstarkov can quickly validate

HenriBeck commented 5 years ago

I will fix the things for the react-jss this evening and then we could release

HenriBeck commented 5 years ago

The issue is fixed in 10.0.0-alpha.8.

https://codesandbox.io/s/xvwlnxk9nw