Re-rendering root node breaks dynamic styles

[WalterWeidner]

Expected behavior:
The root component of a React app that is using react-jss for styles should be able to re-render and not break the styling of its child components.

Describe the bug:
This bug occurs when the root node of a React app re-renders and then later an instance of a child component unmounts when another instance of the same component mounts.

From my research, it looks like this is caused by the indexOf check in RuleList's remove function. The indexOf uses strict equality checks which fails to find a match after the re-render. The rule is no longer strictly equal due to something with how the provider works. This means that you are splicing with a -1 index, effectively removing the wrong items from the list.

['a', 'b', 'c'].splice(-1, 1)
// ['c']

This could be fixed by refactoring that line in RuleList from:

this.index.splice(this.index.indexOf(rule), 1)

to:

const index = this.index.findIndex(v => v.key === rule.key);
if (index > -1) {
    this.index.splice(index, 1)
}

You could also update the provider so that it doesn't return a different object instance.

Reproduction:
You can see an example of this failure on this codepen. In that example one instance of the component is unmounting while another is mounting. The cleanup function for the first instances's dynamic styles runs after the second instance has created its sheet and it removes the new sheet.

Versions (please complete the following information):

• jss: 10.9.2
• Browser [e.g. chrome, safari]: Chrome
• OS [e.g. Windows, macOS]: macOS

This is possibly related to #917, though that issue has been closed.

[fjpedrosa]

I am interested on this. Will you open a PR?

[Bundas]

Hey guys, we have encountered the same problem in our project. Basically, each time the route changed, some styles didn't load properly. For example, if the table got rerendered, the last row would have broken styles. This indeed fixes the problem. I have created a PR

[ernestostifano]

Hi guys, we are currently facing the same (or at least similar) issue in a very big project. In our case, static styles get lost, probably because they are removed under the same conditions explained above. Dynamic styles continue to work because since they are linked to component's props, they get updated/added again.

We will try to debug this further, but everything seems to point to the remove function.

@Bundas @kof do you think the PR will be merged any soon? I will try to patch the dependency and see if our issue gets resolved.

Btw, we are using v10.10.0

[ernestostifano]

@Bundas @kof

Hi again guys, I can confirm that our issue is the same. Applied the modifications of the PR into a patch and everything is now working as expected! 🎉

[Bundas]

@ernestostifano I am glad it's working for you now. Unfortunately, the fix proposed by @WalterWeidner is not addressing the root cause. But it does the trick as a hotfix to get going.

btw for anybody wondering how to apply the patch to their project - we use https://www.npmjs.com/package/patch-package

[ernestostifano]

@Bundas oh, I understand. Is someone already working on it? Is it a priority for you? If needed, I could try to help. For us it turns to be an important issue (all the static styles get lost when it happens).

Regarding the patch, for people using modern versions of yarn, especially with PnP, we used yarn patch.

[ernestostifano]

I must add that in our case, the "root" was not being re-rendered, neither was the theme provider. It was sufficient to unmount just a portion of the app and mount it again, after unmounting/mounting some of the internal components.

[WalterWeidner]

Hey guys, we have encountered the same problem in our project. Basically, each time the route changed, some styles didn't load properly. For example, if the table got rerendered, the last row would have broken styles. This indeed fixes the problem. I have created a PR

Thanks. For some reason I wasn't notified of responses to this issue.

[ernestostifano]

Could be related: #1630

[ernestostifano]

Hi guys, I am not pretending to make anyone fix this, just want to receive proper briefing to try fixing it myself.

Why is this PR (#1642) not good? Insufficient implementation? Missing tests?

I really like JSS, have been using it since the beginning, but now my team is evaluating to migrate to other solutions (like React TSS), because this issue completely breaks our UIs.

This is a friendly comment guys, I would really like to help or at least to understand, because this repo is not having a lot of activity and that is a risk for us (the project is really big).

@WalterWeidner @Bundas @fjpedrosa @kof