cssinjs / jss

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

Cannot read property rules of undefined #1188

Closed vtereshyn closed 4 years ago

vtereshyn commented 4 years ago

Hi. Not sure how to reproduce locally or create a code snippet with a reproducible issue, but got this today 5 times on stage environment. It's a really weird and very important issue since I can't debug it Hope you can help.

Thank you in advance

image

vtereshyn commented 4 years ago

https://github.com/cssinjs/jss/pull/1189 not sure if this PR could fix the issue, but guess the JSS shouldn't get down in any cases.

vtereshyn commented 4 years ago

So, this issue brokes an application in production. Of course its my bad to use package alpha version in production, but here is a result. React-jss is not ready for use in prod builds

HenriBeck commented 4 years ago

Could you please include more info about your setup and under which conditions you are using react-jss so we can look into this?

vtereshyn commented 4 years ago

@HenriBeck I couldn't create a codesandbox with a reproducible example, but I can try to describe:

I have a component with dynamic styles. This is FunctionComponent which uses createUseStyles. There I have a few CSS properties which dynamically change depends of Component props. I use this component on several pages. So when I return from one of the pages to another I got the error described above. In the console, I saw that the problem in onUpdate method or smth like that.

Also, before I had an issue with dynamical styles. My styles didn't update properly. So I needed a full page reload to update CSS. This is weird.

But I'm looking forward to trying to help you in fixing these issues.

Now I resolved these issue using different classnames depends on Component props.

HenriBeck commented 4 years ago

The not updating of styles should be fixed in #1190

vtereshyn commented 4 years ago

Cool, I'll update my jss versions and check. Thanks for the update. Maybe my problem relates to #1190

HenriBeck commented 4 years ago

We haven't released a version with that fix yet

vtereshyn commented 4 years ago

@HenriBeck yes, I know. So I'm waiting for the new version and then update

felthy commented 4 years ago

I've been having this problem too. It won't be fixed by #1190, it's a different problem.

The crash can be triggered by using a dynamic style inside a media query e.g.:

  button: {
    '@media (min-width: 0px)': {
      color: () => 'red'
    }
  }

The crash occurs if you unmount a component that uses this sheet, while there are still other instances mounted.

Here's a codesandbox:

https://codesandbox.io/s/jss-mq-dynamic-style-remove-ju7wr

Just click "Remove" to crash it.

@HenriBeck I think the problem here is that the key on ConditionalRule is not unique for per mounted instance, so when it is removed by removeDynamicRules() there is no longer a rule in RuleList.map with the key @media (min-width: 0px) so a subsequent call to updateDynamicRules() with that key will crash. Other rules e.g. StyleRule have unique keys e.g. button-0.

HenriBeck commented 4 years ago

Also, before I had an issue with dynamical styles. My styles didn't update properly. So I needed a full page reload to update CSS. This is weird.

I was linking your fix to this problem. I'm aware that this problem is completely unrelated to yours

felthy commented 4 years ago

Yes but to clarify, my previous comment is all about this issue. The codesandbox above is new - I created it specifically for this issue because there was no sandbox yet to reproduce this bug.

iamstarkov commented 4 years ago

it affects us too

iamstarkov commented 4 years ago

https://github.com/nordnet/nordnet-ui-kit/pull/649

stoicsleuth commented 4 years ago

Is there any update on this issue?

alex-shamshurin commented 4 years ago

The same is here!

hartbeatnt commented 4 years ago

This is affecting my company's production site as well, any timeline on when a version with the fix will be released?

iamstarkov commented 4 years ago

@hartbeatnt you can submit a PR with a fix and then it will be released shortly after successful review

alex-shamshurin commented 4 years ago

Any update? Does anybody knows where is it ?

kof commented 4 years ago

Seem like a bad bug, does anyone want to create a test or/and a fix?

iamstarkov commented 4 years ago

@kof this can be used for a test https://github.com/nordnet/nordnet-ui-kit/pull/649/files

vladanyes commented 4 years ago

Waiting for the new version too

hi-zhaolei commented 4 years ago

I think I found the reason for this problem.

We met this issue by use react-hot-loader. When some component code change. webpack retranspile and make hot reload. Then the component use jss hook will show this error and will make our program broken down. It makes me really headache.

After some debug. I found the parameter rule is undefined because the property dynamicRules on state not rewritten when state.sheet removes the rules.

For details.

In the picture following. We use addDynamicRules to add rulelist on the sheet and assigned to state.dynamicRules.

image

And in effect clean up callback. Only remove all rules but forgot to reset property dynamicRules on state

image

So. when the next effect hook execute. We can`t found the dynamicRules in rulelist because it`s already removed before. So I try to reset dynamicRules property and it works.

I'm not sure the code above is intentional or it`s a bug. Hope someone can response.

kof commented 4 years ago

Created a minimal failing test for this.

kof commented 4 years ago

fix is merged, will be released in 10.0.1