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

Cannot read property rules of undefined #1188

Closed
vtereshyn opened this issue Sep 10, 2019 · 24 comments
Closed

Cannot read property rules of undefined #1188

vtereshyn opened this issue Sep 10, 2019 · 24 comments
Labels
bug It went crazy and killed everyone. important The thing you do when you wake up!

Comments

@vtereshyn
Copy link
Contributor

vtereshyn commented Sep 10, 2019

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
Copy link
Contributor Author

vtereshyn commented Sep 11, 2019

#1189
not sure if this PR could fix the issue, but guess the JSS shouldn't get down in any cases.

@vtereshyn
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

@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
Copy link
Member

The not updating of styles should be fixed in #1190

@vtereshyn
Copy link
Contributor Author

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

@HenriBeck
Copy link
Member

We haven't released a version with that fix yet

@vtereshyn
Copy link
Contributor Author

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

@felthy
Copy link
Member

felthy commented Sep 16, 2019

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
Copy link
Member

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
Copy link
Member

felthy commented Sep 16, 2019

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.

@kof kof added the bug It went crazy and killed everyone. label Sep 22, 2019
@iamstarkov
Copy link
Member

it affects us too

@iamstarkov
Copy link
Member

@stoicsleuth
Copy link

Is there any update on this issue?

@alex-shamshurin
Copy link

alex-shamshurin commented Oct 21, 2019

The same is here!

@hartbeatnt
Copy link

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

@iamstarkov
Copy link
Member

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

@alex-shamshurin
Copy link

Any update? Does anybody knows where is it ?

@kof kof added the important The thing you do when you wake up! label Nov 24, 2019
@kof
Copy link
Member

kof commented Nov 24, 2019

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

@iamstarkov
Copy link
Member

@vladanyes
Copy link

Waiting for the new version too

@hi-zhaolei
Copy link

hi-zhaolei commented Dec 14, 2019

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
Copy link
Member

kof commented Dec 15, 2019

Created a minimal failing test for this.

@kof
Copy link
Member

kof commented Dec 28, 2019

fix is merged, will be released in 10.0.1

@kof kof closed this as completed Dec 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug It went crazy and killed everyone. important The thing you do when you wake up!
Projects
None yet
Development

No branches or pull requests

10 participants