iamstarkov / jss-increase-specificity

Increases specificity of selectors. Port of postcss-increase-specificity
11 stars 1 forks source link

Dynamic sheet doesn't update because of this plugin #2

Closed iamstarkov closed 7 years ago

iamstarkov commented 7 years ago

Originally reported by @mistadikay in https://github.com/iamstarkov/jss-increase-id-specificity/issues/3
participators: @mistadikay, @kof

Hey!

I'm using react-jss with rules like:

borderColor: ({ isValid }) => (isValid ? '#a5a5a5' : '#f00')

When isValid prop changes I can see that this function executes and returned value is correct, but the sheet doesn't update and visually border-color stays the same.

While debugging this issue I found out that this plugin was the problem, in particular this string in onProcessSheet hook:

rule.selectorText = prefix + rule.selectorText;

My questions are more to @kof I guess: 1) what is the correct way to change selector (assuming that the one described above is incorrect) to make dynamic sheets work? 2) could it be maybe connected with this issue?

Any help appreciated, thanks.

iamstarkov commented 7 years ago

published as jss-increase-specificity@0.2.0

mistadikay commented 7 years ago

The problem still persists. Updated reproducible case with jss@8 and react-jss@7: https://www.webpackbin.com/bins/-KpdPv8lAXfOMcVxO83V

iamstarkov commented 7 years ago

that is interesting, i'll take a look

mistadikay commented 7 years ago

apparently the bug can be reproduced even without this plugin. I just manually put the prefix:

input: {
  ':not(#\\20)&': {
    borderColor: ({ isValid }) => isValid ? '#ccc' : '#f00'
  }
}

And border color does not update on isValid prop change. Any ideas why @kof?

Updated example: https://www.webpackbin.com/bins/-KqNQW0EgULbR0SiYk8Z

kof commented 7 years ago

Not from looking at it, there are too many dependencies involved. Can you reproduce it with jss core only? Ideally submit a failing test?

kof commented 7 years ago

as you are using ampersand, it might be also jss-nested fault

mistadikay commented 7 years ago

Removed everything except jss-nested and react-jss — not sure how to reproduce it without them: https://www.webpackbin.com/bins/-KqNVNfzLuuYxwDi5cY_

kof commented 7 years ago

sheet.update(data) is the method which is called by react-jss

mistadikay commented 7 years ago

Removed react-jss: https://www.webpackbin.com/bins/-KqNVNfzLuuYxwDi5cY_ Nor sure if I'm updating correctly:

sheet.update({ isValid });
const { classes } = sheet.attach();
kof commented 7 years ago

border is red in your example, and it says it should be red.

mistadikay commented 7 years ago

when you change the value, it says it should be grey, but it stays red

kof commented 7 years ago

Strange, it is reproducible without jss-nested as well. https://www.webpackbin.com/bins/-KqNhpnr4ZAJ82kywzI2

iamstarkov commented 7 years ago

what do we do?

kof commented 7 years ago

Need to reproduce iit with a test and fix, if you are interested I can handle it over to you

kof commented 7 years ago

Just looked at it again, we forgot to add option link: true, now it works https://www.webpackbin.com/bins/-KqNhpnr4ZAJ82kywzI2

kof commented 7 years ago

So the issue is related to that :notthing, if you use just '&' it works.

kof commented 7 years ago

:not(.bla)& works as well, so it is because of #\\20

kof commented 7 years ago

I have a suspicion that we are not linking correctly the css rule and jss rule because of escapings, because they don't match

kof commented 7 years ago

tbc https://github.com/cssinjs/jss/issues/557

iamstarkov commented 7 years ago

in short: the rabbit hole is quite deep

kof commented 7 years ago

should be fixed in jss@9.0.0-pre.1 @mistadikay let me know

mistadikay commented 7 years ago

doesn't look like it? But I'm not sure how to test it without involving jss-nested https://www.webpackbin.com/bins/-KqNVNfzLuuYxwDi5cY_

kof commented 7 years ago

You bin link didn't load, I tested here, works for me https://www.webpackbin.com/bins/-KqgiTDVbilzhS2C8xaD

mistadikay commented 7 years ago

Github lost underscore from the link. 🙄 Fixed.

I noticed you're passing {link: true} as a second argument to createStyleSheet — what is it?

kof commented 7 years ago

http://cssinjs.org/js-api?v=v9.0.0-pre.1#create-style-sheet

It is required to be true in order to activate function values right now. Not sure if we should change that in the future but lets discuss that in a separate issue.

mistadikay commented 7 years ago

I guess it's going to be turned on by default in react-jss?

kof commented 7 years ago

For dynamic sheets yes.

kof commented 7 years ago

folow this one https://github.com/cssinjs/jss/issues/557