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

Media query styles applied only to the first element in function #1320

Open
karpcs opened this issue Mar 25, 2020 · 26 comments · May be fixed by #1343
Open

Media query styles applied only to the first element in function #1320

karpcs opened this issue Mar 25, 2020 · 26 comments · May be fixed by #1343
Assignees
Labels
feature request This will safe many lifes! help wanted

Comments

@karpcs
Copy link

karpcs commented Mar 25, 2020

Expected behavior:
when changing screen size media query in function should apply styles to all the component that uses it

Describe the bug:
only the first component gets the styles of media query in function, it looks like it was introduced in 10.0.1

Codesandbox link:
https://codesandbox.io/s/react-jss-playground-nwurq

Versions (please complete the following information):

  • jss:10.1.1
  • Browser saw it on chrome, havent tested others
  • OS windows10:

image

@pranshuchittora
Copy link
Contributor

@kof if this issue is still open then I would like to work on this :)

@kof
Copy link
Member

kof commented May 5, 2020

@pranshuchittora sure

@pranshuchittora
Copy link
Contributor

Thanks @kof for the blazing fast reply 👍

@pranshuchittora
Copy link
Contributor

-  wrapper: () => ({
    padding: 40,
    background: theme.background,
    textAlign: "left",
    "@media (min-width: 1024px)": {
      backgroundColor: "red"
    }
-  }),
+ wrapper: {
    padding: 40,
    background: theme.background,
    textAlign: "left",
    "@media (min-width: 1024px)": {
      backgroundColor: "red"
    },
+  }, 

@karpcs Converting the style object from an arrow function to plain object works.

Screenshot from 2020-05-05 19-32-45

@kof probably this is happening due to improper object referencing in arrow function approach. Is it still a bug, if no then I suggest closing this issue :)

@kof
Copy link
Member

kof commented May 5, 2020

Feel free to dig deeper and figure out what the actual bug is. Yes it is only when rule function is used.

@kof
Copy link
Member

kof commented May 5, 2020

Same as #1327

@pranshuchittora
Copy link
Contributor

Rule function is not transpiling ?
Screenshot from 2020-05-05 20-25-04

@kof
Copy link
Member

kof commented May 5, 2020

Might be just the playground, also it's not on the latest version, feel free to update and check what it does.

@pranshuchittora
Copy link
Contributor

I have updated the deps.

    "jss": "^10.1.1",
    "jss-preset-default": "^10.1.1"

Screenshot from 2020-05-05 20-47-02

Still not working, will take a look at the rule function parsing.
Is this the correct function to look at ?

@pranshuchittora
Copy link
Contributor

Hi @kof , Did some preliminary research and found out that the function is not being executed to get the returned object as shown below.

Screenshot from 2020-05-06 03-22-26

As the function is not bein executed, therefore converting it toCss string returns null.

@karpcs
Copy link
Author

karpcs commented May 6, 2020

I kept the version in playground at the version, that this bug appears, prior versions does not have this bug.

i noticed this when looping through array and passing different props to each and only the first element got the styles, so i just put a little demo here

@kof
Copy link
Member

kof commented May 6, 2020

@pranshuchittora yes, you are still on the surface, sheet.update is supposed to call function rules, jss-plugin-rule-value-function plugin is responsible for implementing the function rule API ... somewhere between jss core and the plugin there is a bug, most likely in the plugin.

@kof
Copy link
Member

kof commented May 6, 2020

https://github.com/cssinjs/repl/blob/gh-pages/src/index.js#L55 repl doesn't call .update, so on the repl function rules won't work, we need to start calling .update there.

The actual issue in your production code might be in react-jss

@pranshuchittora
Copy link
Contributor

@pranshuchittora yes, you are still on the surface, sheet.update is supposed to call function rules, jss-plugin-rule-value-function plugin is responsible for implementing the function rule API ... somewhere between jss core and the plugin there is a bug, most likely in the plugin.

Yeah, after call update() it worked. I have raised a PR regarding the following fix
cssinjs/repl#5

@pranshuchittora
Copy link
Contributor

Till v10.0.0 it was working perfectly. In v10.0.1 the issue of dynamicStyles is occurring.

The line ->

const ruleStr = rule.toString()

Is trying to concert the dynamic style to string, which is unable to.

Screenshot from 2020-05-07 01-34-12

This flow is encountered when the link is true.
IMO the issue is in jss not react-jss.

P.S. update method on rule is not available.

@kof
Copy link
Member

kof commented May 6, 2020

That line was changed last time before 10.0.0

@pranshuchittora
Copy link
Contributor

Hi @kof ,
So I ran git bisect between the v10.0.0 & v10.0.1. And found that the commit which introduced that bug was 9f7924eb79bf1e78816db7aaa4cf324471.

Screenshot from 2020-05-09 14-25-42

Reverting the changes in packages/react-jss/src/createUseStyles.js introduced in the following commit fixes the bug.

@kof
Copy link
Member

kof commented May 9, 2020 via email

@pranshuchittora
Copy link
Contributor

@kof kindly check #1343 for the fixes :)

@jmikrut
Copy link

jmikrut commented Apr 4, 2021

This is still an issue, correct? I am on 10.6.0 and can reproduce.

@kof
Copy link
Member

kof commented Apr 4, 2021

#1343 (comment)

@mjeanroy
Copy link

mjeanroy commented Jun 23, 2021

Hi @kof,

I tried to debug this, and I would like to share with you what I found.
With my coworker @pybuche, we tried to understand what was the issue here, and he found something interesting: if we wrap the call to manageSheet in a setTimeout, the issue disappears!
We tried to understand why, and here is my explanation:

  1. When the first component renders, everything works fine. In fact, the sheet is correct, and all the rules are deployed using manageSheet, the sheet is then marked as attached.

  2. When the second component renders, the sheet is already attached, but all its dynamic rule are updated.

If we wrap manageSheet in a setTimeout, then, when the second component renders, the sheet is not attached yet to the DOM, so it works because all the rules are attached & deployed properly.

The issue really occurs when updating a dynamic rule, in fact, if we do:

if (newSheet) {
  // Detach the sheet so all rules will be inserted
  unmanageSheet({
    index,
    context,
    sheet: newSheet,
    theme
  });

  manageSheet({
    index,
    context,
    sheet: newSheet,
    theme
  });
}

=> It also fixes the issue.

I tried to go deeper to understand why updating dynamic rules did not works as exepected, and I think the issue is in jss-plugin-nested, especially here:

} else if (isNestedConditional) {
  // Place conditional right after the parent rule to ensure right ordering.
  container
    .addRule(prop, {}, options)
    // Flow expects more options but they aren't required
    // And flow doesn't know this will always be a StyleRule which has the addRule method
    // $FlowFixMe[incompatible-use]
    // $FlowFixMe[prop-missing]
    .addRule(styleRule.key, style[prop], {selector: styleRule.selector})
}

The line container.addRule insert rule, but in fact, the rule is not yet up to date, since the child rule is added the line after (to summarize, insertRule happens too early I think).

I tried to fix it by splitting the rule addition and the insertion, something like this:

const addedRule = container.createRule(prop, {}, options);
addedRule.addRule(styleRule.key, style[prop], {selector: styleRule.selector})
container.deployRule(addedRule);

You can find the commit on my fork here. Note that it is just a POC, I did not try to make it "super clean", but if you think the solution seems OK, feel free to give your opinion and I can work on a PR :)

In the meantime, I would like to suggest a workaround until the issue is fixed: adding a plugin that detach and re-attach the sheet, such as:

jss.use({
  onUpdate(data: any, rule, sheet, options) {
    if (sheet) {
      sheet.detach().attach();
    }
  },
});

This workaround can definitely be improved, that is just the basic idea :)

Also, thanks for your amazing library, that's an impressive work, and I really love using it

[EDIT] I opened #1523 to get your feedback

@mjeanroy mjeanroy mentioned this issue Jun 23, 2021
2 tasks
@hkar19
Copy link

hkar19 commented Jul 10, 2021

definitely @mjeanroy suggestion to use:

jss.use({
  onUpdate(data: any, rule, sheet, options) {
    if (sheet) {
      sheet.detach().attach();
    }
  },
});

is very nice.

I just want to add, if you are using react-jss, remember that using jss.use means you need to do your custom setup.

therefore, you need to do things manually such as:

  1. dont forget to call preset from jss-preset-default in your createJss, such as:
import { create as createJss, Rule, StyleSheet } from "jss";
import { JssProvider, ThemeProvider } from "react-jss";
import preset from "jss-preset-default";

// i made my array of plugins here, and spread it in my jss.use
import jssPlugins from "./jssPlugins";

// dont forget this!
const jss = createJss(preset());
jss.use(
  {
    onUpdate(data: object, rule: Rule, sheet: StyleSheet) {
      if (sheet) {
        sheet.detach().attach();
      }
    },
  },
  ...jssPlugins
);

ReactDOM.render(
  <React.StrictMode>
    <JssProvider jss={jss}>
      <ThemeProvider theme={theme}>
        <App />
      </ThemeProvider>
    </JssProvider>
  </React.StrictMode>,
  document.getElementById("root")
);
  1. in jssPlugins.ts, i do:
import a from "jss-plugin-camel-case";
import b from "jss-plugin-compose";
import c from "jss-plugin-default-unit";
import d from "jss-plugin-expand";
import e from "jss-plugin-extend";
import f from "jss-plugin-global";
import g from "jss-plugin-nested";
import h from "jss-plugin-props-sort";
import i from "jss-plugin-rule-value-function";
import j from "jss-plugin-rule-value-observable";
import k from "jss-plugin-template";
import l from "jss-plugin-vendor-prefixer";
// add any more plugins you desire

export default [a(), b(), c(), d(), e(), f(), g(), h(), i(), j(), k(), l()];

this workaround is tedious, but for now this is all we got


UPDATE: actually this method will give some annoyance in storybook where some styles got lost if you are using style as function. if you're using style as object, it would be working fine.

for example:
this will be fine

content: {
      flex: 1,
      overflowY: "scroll",
      // overflowX: ""
      maxHeight: "100vh",
      backgroundColor: theme.defaultBackground,
    }

some of these style will be lost somehow, (only in storybook, btw).

content: ()=>({
      flex: 1,
      overflowY: "scroll",
      // overflowX: ""
      maxHeight: "100vh",
      backgroundColor: theme.defaultBackground,
    })

@j4mesjung
Copy link

Hello, any updates on a fix for this? Experiencing this issue on 10.9.0

@TchernyavskyDaniil
Copy link

Sorry if I offended or don't understand your processes, I'm speaking as a regular consumer of your library

This bug is from version 10.0.1. Shouldn't it be fixed by the current library team? Part of my team is giving up on jss, as well as some people I know because of this problem. I honestly don't quite understand the point of preparing a move to 18 React if there are such critical bugs

@kof
Copy link
Member

kof commented Apr 19, 2022

I am merging good PRs that make progress, if a PR ads support for react 18 and has tests etc - cool.

if you want to get something fixed, please fix it, add a test - I will merge it too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request This will safe many lifes! help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants