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

Version 10.8.1 release looks incompatible with 10.8.0. #1565

Open
mfaheemakhtar opened this issue Oct 19, 2021 · 20 comments
Open

Version 10.8.1 release looks incompatible with 10.8.0. #1565

mfaheemakhtar opened this issue Oct 19, 2021 · 20 comments

Comments

@mfaheemakhtar
Copy link

Expected behavior:
Things should work.

Describe the bug:
I am using MUI v5 and using a theme that is using jss in style provider. The theme uses @mui/styles which is depreciated for mui5 but was working fine. I am also using data-grid.

The recent release of 10.8.1 caused the app to break when a screen with data-grid is viewed. I spent multiple hours to find that there are different versions of jss and plugins in the yarn.lock, and 10.8.0 and 10.8.1 does not seem to go together because of replaceUrl or something.

Did some tweaking but then there was a type issue. I don't know where the problem is and I could not find anything so I am posting it here. The problem is probably not with the package itself but a unique combination of different packages with different versions of jss and plugins.

Reproduction:

Unfortunately, I was not able to reproduce it outside of the project. May try on weekends if got time.

Versions (please complete the following information):

  • jss: 10.8.1
  • Browser [e.g. chrome, safari]: Brave
  • OS [e.g. Windows, macOS]: Windows 10 Pro, Ubuntu 20.x
  • @mui/styles: 5.0.0
  • @mui/x-data-grid-pro": "^5.0.0-beta.2"
    Feel free to add any additional versions which you may think are relevant to the bug.

Managing expectations:

Maintainers will not be fixing the problem you have unless they have it too, if you want it to get fixed:

  1. Submit a PR with a failing test
  2. Discuss a solution
  3. Implement it

You can also do the first step only and wait for someone else to work on a fix. Anything is much better than nothing.

I am short of time at the moment and can't really provide anything right now, maybe I can try to reproduce on weekends if I get time.

The yarn resolutions fixed the issue for me:

"resolutions": {
    "jss": "10.8.0",
    "jss-plugin-camel-case": "10.8.0",
    "jss-plugin-default-unit": "10.8.0",
    "jss-plugin-global": "10.8.0",
    "jss-plugin-nested": "10.8.0",
    "jss-plugin-props-sort": "10.8.0",
    "jss-plugin-rule-value-function": "10.8.0",
    "jss-plugin-vendor-prefixer": "10.8.0"
  }

If someone else face the similar issue, try this.

@viko16
Copy link

viko16 commented Oct 20, 2021

+1

10.8.1 changed the output of nested styles, now it is can't generate correct nested and dynamic className

That's my case:
https://codesandbox.io/s/amazing-rain-ut7q1?file=/src/App.js

@kabdelkareem
Copy link

+1 we have the same issue.
it caused issues in material-ui styles

@seiyab
Copy link
Member

seiyab commented Oct 20, 2021

Extending number of items to 10, #1565 (comment) shows 2nd, 5th, and 8th items render wrong style.
react-jss might accidentally generates same key for different rules and they are replaced by update of other rules.

@kof
Copy link
Member

kof commented Oct 21, 2021

@seiyab do you see an easy fix?

@seiyab
Copy link
Member

seiyab commented Oct 21, 2021

@kof Not yet. I would read react-jss codes.
I found that react-jss might still cause memory leak.
I peeked RuleList that #1565 (comment) generates.
It generates more rules on rerendering component.
I should have added rerendering test for react-jss.

@kof
Copy link
Member

kof commented Oct 21, 2021

Let me know if we should revert the change for now and release a previous version as a patch

@mfaheemakhtar
Copy link
Author

mfaheemakhtar commented Oct 21, 2021

@seiyab, not sure if it is helpful but the style of tiem number 2 in @viko16 is missing because of this line in jss removes the style from the cssRules.

//_proto.replaceRule
this.cssRules.splice(index, 1);

But this line only inserts the new rule in the container styles and not in the cssRules

return this.insertRule(rule, index);

This makes the _proto.refCssRule function to replace the wrong item in the array because the element to be removed is not inserted yet?

if (rule.options.parent instanceof StyleSheet) {
      this.cssRules[index] = cssRule;
}

TLDR:
The old rule was at index 4.
Splice removed it. Now next style (of item 2) is at index 4.
Insert should add the new rule in cssRule at index 4, So style of 2 gets back to 5 again.
At last style at index 4 (which was 5 initially) gets replaced by new rule.

Potential Solutions:

  1. Don't remove from this.cssRules because it will eventually get replaced later. I don't know it will work.

My breakpoints
image

@seiyab
Copy link
Member

seiyab commented Oct 21, 2021

It seems to be better to revert 10.8.1 because #1565 is more serious than #1360 .

@mfaheemakhtar Thanks a lot. It will help.

@seiyab
Copy link
Member

seiyab commented Oct 22, 2021

Perhaps

this.cssRules[index] = cssRule

should be

this.cssRules.splice(index, 0, cssRule)

However, it still render rules in different order between RuleList and style.
I use following test case and it went wrong.

sheet = jss
  .createStyleSheet(
    {
      a: ({color}) => ({
        color: 'red',
        '& a': {
          color
        }
      }),
      b: ({display}) => ({
        display: 'block',
        '& b': {
          display
        }
      })
    },
    {link: true}
  )
  .attach()

@mfaheemakhtar
Copy link
Author

I will do some more debugging tomorrow and compare 10.8.0 and 10.8.1. I will also take a look at what changed in tests.

@kof
Copy link
Member

kof commented Oct 22, 2021

please somebody add a failing test for this new problem

seiyab added a commit to seiyab/jss that referenced this issue Oct 23, 2021
seiyab added a commit to seiyab/jss that referenced this issue Oct 23, 2021
@seiyab
Copy link
Member

seiyab commented Oct 23, 2021

I submitted PR #1571 that adds a failing test for this problem.

Additionally, I wrote behavior for each version in branch for experiment. https://github.com/seiyab/jss/blob/67eb962900910a4d675f71480d6285fe19dfdc04/packages/react-jss/src/createUseStyles.test.js#L79-L282
Both v10.8.0 and v10.8.1 renders wrong style. and v10.8.1 is more serious.
v10.8.0: generates more and more rules on update.
v10.8.1: rules can be replaced by another rule on update.

Replacing this.cssRules[index] = cssRule by this.cssRules.splice(index, 0, cssRule), perhaps it is resolved.
https://github.com/seiyab/jss/blob/67eb962900910a4d675f71480d6285fe19dfdc04/packages/react-jss/src/createUseStyles.test.js#L284-L370

@kof
Copy link
Member

kof commented Oct 24, 2021

if this.cssRules.splice(index, 0, cssRule) is fixing the problem, should we release the fix then rather than reverting?

@seiyab
Copy link
Member

seiyab commented Oct 25, 2021

In my opinion, reverting as first aid is a safer option.
Reasons:

@kof
Copy link
Member

kof commented Oct 25, 2021

I see, alright, reverting now

kof added a commit that referenced this issue Oct 25, 2021
@kof
Copy link
Member

kof commented Oct 25, 2021

Patch released https://github.com/cssinjs/jss/releases/tag/v10.8.2

kof pushed a commit that referenced this issue Oct 26, 2021
* add test for accidental replace in v10.8.1

Issue: #1565

* revert unwanted format
seiyab added a commit to seiyab/jss that referenced this issue Oct 29, 2021
This reverts commit 45f4463.

try to fix memory leak again
seiyab added a commit to seiyab/jss that referenced this issue Oct 29, 2021
This reverts commit 45f4463.

try to fix memory leak again
@seiyab seiyab mentioned this issue Oct 29, 2021
2 tasks
kof pushed a commit that referenced this issue Dec 8, 2021
This reverts commit 45f4463.

try to fix memory leak again
@ashkanahrabi
Copy link

ashkanahrabi commented Dec 13, 2021

I'm facing this issue again with jss version 10.8.2.

Some styles are not available in the document and after refreshing the page, everything's ok.

There may be a conflict with MUI styles, as you can see below there are two card classes:

Here's the element:
Screen Shot 2021-12-13 at 13 00 32

Styles:
Screen Shot 2021-12-13 at 13 02 29

and after page refresh:

Screen Shot 2021-12-13 at 13 03 23

any ideas @kof? I also updated to 1.9.0 but nothing changed :(

@kof
Copy link
Member

kof commented Dec 13, 2021

@ashkanahrabi can you create a minimal reproducible version ideally on codesandbox?

@seiyab
Copy link
Member

seiyab commented Feb 2, 2022

MUI closed mui/mui-x#2993 that is caused by this issue.

@kof
Copy link
Member

kof commented Feb 2, 2022

Is this still the case with 10.9.0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants