fix 1360

[Jokcy]

Corresponding issue (if exists):

#1360

What would you like to add/fix?

fix memory leak with dynamic styles

[Jokcy]

Demo here https://codesandbox.io/s/awesome-faraday-uufqb?file=/jss.js:61976-65667

[Jokcy]

It seems some error with CI config

[kof]

proposal: lets make a new method - replaceRule which will do exactly this?

[Jokcy]

you mean, we add relpaceRule in StyleSheet:

replaceRule(name: string, decl: JssStyle, options?: RuleOptions): Rule | null {
  if (options) {
    options.safeReplace = true
  }
  else {
    options = { safeReplace: true }
  }

  this.addRule(name, decl, options)
}

or just implement a same addRule method expect safeReplace logic ?

[kof]

or just implement a same addRule method expect safeReplace logic ?

this, yeah

[kof]

it seems to me that it would be very obvious semantics

[Jokcy]

But if we do that, which not add safeReplace option, we may have to add a replace method to RuleList. That adds a lot of extra code.

[Jokcy]

or maybe we can implement addOrReplace to share the same logic

[Jokcy]

I will have a try

[kof]

I def. don't want to add a lot of extra code, it should share the logic

[Jokcy]

@kof I updated code to use replaceRule, pleace review the code. And I have to update two test cases in React-JSS

[kof]

it broke the order it seems, otherwise why is this diff here?

[Jokcy]

if you don't konw the reason, I guess I need to go deep to find out the reason

[kof]

yeah, can't tell just by looking at this changes

[Jokcy]

I found the reason, it because React-jss use sheet.addRule to handle dynamic rules, in this case, it will add:

container: {
          display: 'block',
          '@media (min-width: 0px)': {
            color: ({color}) => color
          }
        }

because it directly using addRule so container will be added as container-d0, then jss will processStyle which:

{
          display: 'block',
          '@media (min-width: 0px)': {
            color: ({color}) => color
          }
        }

then using replaceRule to @media (min-width: 0px) since it is a NestedConditional rule, so it won't increase the counter of RuleList.

In the code before, we use addRule in nest plugin, so the dynamic @media (min-width: 0px) will increase the counter.

Now I'm wondering if it is safe to use replaceRule for all case, even if I can pass all the test cases.

[kof]

Where do you see the danger here? I might not be understanding the problem correctly.

[Jokcy]

let key = name
    if (!isReplace && name in this.raw) {
      key = `${name}-d${this.counter++}`
    }

    this.raw[key] = decl

in this case, if we use old code, this.raw will be:

{
  container: {},
  @media (min-width: 0px): {},
  container-d0: {},
  @media (min-width: 0px)-d1: {},
}

the new add rule won't replace old rule.

The main problem is, if people use addRule to add a nest rule, then it will use replaceRule inside. I think if people want to add rule, it should be really add but not replace.

[kof]

Can you please add a test that reproduces the "wrong" behavior in react-jss? So we can think of better ways of fixing it.

[kof]

the idea of adding a new method replaceRule was to completely avoid passing an option ala isReplace, because it adds a bit of indirection

[Jokcy]

Ah, I finally get your point...

[tokiroto]

There seem no merge due to tests failed. Any progress with that?

[kof]

No this got stuck because we need a test, @Jokcy do you have time to add it?

[ohazda]

Looks like this bug causes lots of issues. Hope you can get it merged soon. 🙏🏻

[kof]

This is not gonna be merged unless we have a test that fails with the current behavior. Once someone creates a test like that we can validate this fix and some other ideas of how to fix it. Feel free to create separate PR if needed with just a failing test.

[tokiroto]

Any update on merge?

[kof]

We NEED a test like mentioned 2 times above. Without a test we can not merge this PR. Anyone feel free to step in and write a test for the wrong behaviour that causes the bug. It seems like we should write a few unit tests that uses the API the same way react-jss does like described here https://github.com/cssinjs/jss/pull/1381/files#r470438013

Or alternatively write a test in react-jss that reproduces this behaviour.

[seiyab]

I wrote a test for this problem.
And unfortunately, my test shows that this PR does NOT resolve the issue.
This might be my failure because @Jocky wrote a demo #1381 (comment) and they should have verified that it resolved the issue.
Now we cannot access their demo code.
Need help.

This is the test, that fails.
https://github.com/seiyab/jss/blob/16346eaaa627a9c66b9ec096e48968227e903a00/packages/react-jss/tests/dynamicStyles.js#L44-L93

[kof]

I think we got this finally addressed in #1563