search

cssinjs / jss

JSS is an authoring tool for CSS which uses JavaScript as a host language.
https://cssinjs.org
MIT License
7.06k stars 397 forks source link

A strange off-by-one problem – incorrect style is applied sometimes #926

Closed farcaller closed 5 years ago

farcaller commented 5 years ago

I'm trying to debug a problem where material-ui-kit pulls in material-ui that uses jss and I use it all with gatsby's SSR.

I'll work on a minimal reproducible build but what I see insofar is that sometimes jss is rendered incorrectly and all the elements get assigned jss+1 indices, exploding the page at random. It seems to be pretty consistent in that +1, e.g.

image

is a good render, while

image

is a bad one.

While I hunt for what exactly triggers the bad behavior, any ideas what could possibly cause this N+1?

npm ls|grep jss
│ ├─┬ @types/jss@9.5.7
│ ├─┬ jss@9.8.7
│ ├─┬ jss-camel-case@6.1.0
│ ├── jss-default-unit@8.0.2
│ ├── jss-global@3.0.0
│ ├─┬ jss-nested@6.0.1
│ ├── jss-props-sort@6.0.0
│ ├─┬ jss-vendor-prefixer@7.0.0
└─┬ react-jss@8.6.1
  ├── jss@9.8.7 deduped
  ├─┬ jss-preset-default@4.5.0
  │ ├── jss-camel-case@6.1.0 deduped
  │ ├─┬ jss-compose@5.0.0
  │ ├── jss-default-unit@8.0.2 deduped
  │ ├── jss-expand@5.3.0
  │ ├─┬ jss-extend@6.2.0
  │ ├── jss-global@3.0.0 deduped
  │ ├── jss-nested@6.0.1 deduped
  │ ├── jss-props-sort@6.0.0 deduped
  │ ├─┬ jss-template@1.0.1
  │ └── jss-vendor-prefixer@7.0.0 deduped
kof commented 5 years ago

In your example you are using createGenerateClassName from MUI, not from react-jss. One of the reasons could be that the way class names generator is used differently on the client and server. We need to see some working code to understand exactly how.

cc @oliviertassinari

oliviertassinari commented 5 years ago

@kof I get issues like this one all the time on Material-UI. We have been documenting the workarounds in the documentation, it's a bit better now. But people are struggling with the index counter approach. I have been personally hit multiple times by the problem. It doesn't leave any room for mistake. I wish we can explore a hash-based generation logic in the future.

HenriBeck commented 5 years ago

So we can close this @oliviertassinari?

oliviertassinari commented 5 years ago

@HenriBeck What's the solution?

farcaller commented 5 years ago

I'm struggling to make a minimal code sample to repro it yet, will get back to it on Monday.

What's confusing is that depending on the imports in files, maybe the import order, or the Moon phase it gets the indices right. At times.

farcaller commented 5 years ago

@oliviertassinari can you point me to a workaround please?

oliviertassinari commented 5 years ago

@farcaller This is a start: https://material-ui.com/guides/server-rendering/#troubleshooting. You can also check the Gatsby example on Material-UI side. I was able to configure it correctly (it's hard).

HenriBeck commented 5 years ago

@oliviertassinari there is a working solution for this, correct?

@farcaller Do you know where the jss1 style is in the bad example? It might be a rule which is conditionally rendered which causes the n+1.

oliviertassinari commented 5 years ago

there is a working solution for this, correct?

@HenriBeck Yes, I hope so. I'm also eager to see if we can find a simple solution.

kof commented 5 years ago

I will do more benchmarking soon, our last approach showed in one bench astonishing results.

On Sat, Oct 27, 2018, 08:53 Olivier Tassinari notifications@github.com wrote:

@kof https://github.com/kof I get issues like this one all the time on Material-UI. People are struggling with the index counter approach. I have been personally been hit multiple time by the issue. It doesn't leave any room for mistake. I wish we can explore a hash-based generation logic in the future.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cssinjs/react-jss/issues/306#issuecomment-433632119, or mute the thread https://github.com/notifications/unsubscribe-auth/AADOWLIHX0lncRh97HCdEu_VVF65CHZWks5upIFvgaJpZM4X9Dxa .

farcaller commented 5 years ago

@farcaller Do you know where the jss1 style is in the bad example? It might be a rule which is conditionally rendered which causes the n+1.

Nowhere, actually.

oliviertassinari commented 5 years ago

Another example: https://github.com/mui-org/material-ui/issues/13355. This time react-loadable is doing black magic behind the scene.

iamstarkov commented 5 years ago

the same happens to us =(

oliviertassinari commented 5 years ago

@iamstarkov @farcaller Do you have a reproduction example :) ?

farcaller commented 5 years ago

@oliviertassinari so, here's the smallest (bleh) demo I came up with so far: https://github.com/farcaller/react-jss-bug-306. 

npm i && rm -rf public/ .cache/ && gatsby build && python3 -m http.server --directory public/

should demonstrate the problem.

I've also nailed it to a single change that fixes the layout (note that the change is done on another page and not on the index one): https://github.com/farcaller/react-jss-bug-306/pull/1

iamstarkov commented 5 years ago

@oliviertassinari we have it in prod and it is not easy to extract minimal repro case

iamstarkov commented 5 years ago

although we can try

iamstarkov commented 5 years ago

we tried material-ui's approach with standalone createClassName generator, but it didn't help

iamstarkov commented 5 years ago

and its odd, because it permanently happens only to 3 pages out of 20+

iamstarkov commented 5 years ago

@farcaller, I can't build it.

  1. I dont have Gatsby installed globally

  2. when I used local one ./node_modules/.bin/gatsby build first time, I got:

    error Generating JavaScript bundles failed

Error: ./src/components/page_chrome.js
Module not found: Error: Can't resolve 'assets/scss/material-kit-react.css' in '/Users/vlasta/projec  ts/oss/react-jss-bug-306/src/components'
resolve 'assets/scss/material-kit-react.css' in '/Users/vlasta/projects/oss/react-jss-bug-306/src/co  mponents'
Parsed request is a module
using description file: /Users/vlasta/projects/oss/react-jss-bug-306/package.json (relative path:  ./src/components)
  aliased with mapping 'assets': '/Users/vlasta/projects/oss/react-jss-bug-306/thirdparty/material  -kit-react-v1.3.0/src/assets' to '/Users/vlasta/projects/oss/react-jss-bug-306/thirdparty/material-k  it-react-v1.3.0/src/assets/scss/material-kit-react.css'
    using description file: /Users/vlasta/projects/oss/react-jss-bug-306/package.json (relative pa  th: ./src/components)
      Field 'browser' doesn't contain a valid alias configuration
      using description file: /Users/vlasta/projects/oss/react-jss-bug-306/thirdparty/material-kit  -react-v1.3.0/package.json (relative path: ./src/assets/scss/material-kit-react.css)
        no extension
          Field 'browser' doesn't contain a valid alias configuration
          /Users/vlasta/projects/oss/react-jss-bug-306/thirdparty/material-kit-react-v1.3.0/src/as  sets/scss/material-kit-react.css doesn't exist
        .mjs
          Field 'browser' doesn't contain a valid alias configuration
          /Users/vlasta/projects/oss/react-jss-bug-306/thirdparty/material-kit-react-v1.3.0/src/as  sets/scss/material-kit-react.css.mjs doesn't exist
        .js
          Field 'browser' doesn't contain a valid alias configuration
          /Users/vlasta/projects/oss/react-jss-bug-306/thirdparty/material-kit-react-v1.3.0/src/as  sets/scss/material-kit-react.css.js doesn't exist
        .jsx
          Field 'browser' doesn't contain a valid alias configuration
          /Users/vlasta/projects/oss/react-jss-bug-306/thirdparty/material-kit-react-v1.3.0/src/as  sets/scss/material-kit-react.css.jsx doesn't exist
        .wasm
          Field 'browser' doesn't contain a valid alias configuration
          /Users/vlasta/projects/oss/react-jss-bug-306/thirdparty/material-kit-react-v1.3.0/src/as  sets/scss/material-kit-react.css.wasm doesn't exist
        .json
          Field 'browser' doesn't contain a valid alias configuration
          /Users/vlasta/projects/oss/react-jss-bug-306/thirdparty/material-kit-react-v1.3.0/src/as  sets/scss/material-kit-react.css.json doesn't exist
        as directory
          /Users/vlasta/projects/oss/react-jss-bug-306/thirdparty/material-kit-react-v1.3.0/src/as  sets/scss/material-kit-react.css doesn't exist
[/Users/vlasta/projects/oss/react-jss-bug-306/thirdparty/material-kit-react-v1.3.0/src/assets/scss/m  aterial-kit-react.css]
[/Users/vlasta/projects/oss/react-jss-bug-306/thirdparty/material-kit-react-v1.3.0/src/assets/scss/m  aterial-kit-react.css.mjs]
[/Users/vlasta/projects/oss/react-jss-bug-306/thirdparty/material-kit-react-v1.3.0/src/assets/scss/m  aterial-kit-react.css.js]
[/Users/vlasta/projects/oss/react-jss-bug-306/thirdparty/material-kit-react-v1.3.0/src/assets/scss/m  aterial-kit-react.css.jsx]
[/Users/vlasta/projects/oss/react-jss-bug-306/thirdparty/material-kit-react-v1.3.0/src/assets/scss/m  aterial-kit-react.css.wasm]
[/Users/vlasta/projects/oss/react-jss-bug-306/thirdparty/material-kit-react-v1.3.0/src/assets/scss/m  aterial-kit-react.css.json]
@ ./src/components/page_chrome.js 2:0-44
@ ./src/pages/404.js
@ ./.cache/async-requires.js
@ ./.cache/production-app.js

  3. when I used local one ./node_modules/.bin/gatsby build following times, I got:

    success open and validate gatsby-configs — 0.010 s
error value must be an array of bytes

TypeError: value must be an array of bytes

- v35.js:29 generateUUID
[react-jss-bug-306]/[uuid]/lib/v35.js:29:38

- create-node-id.js:16 createNodeId
[react-jss-bug-306]/[gatsby]/dist/utils/create-node-id.js:16:10

- load.js:131 processPlugin
[react-jss-bug-306]/[gatsby]/dist/bootstrap/load-plugins/load.js:131:13

- load.js:148 config.plugins.forEach.plugin
[react-jss-bug-306]/[gatsby]/dist/bootstrap/load-plugins/load.js:148:20

- Array.forEach

- load.js:147 module.exports
[react-jss-bug-306]/[gatsby]/dist/bootstrap/load-plugins/load.js:147:20

- index.js:56
[react-jss-bug-306]/[gatsby]/dist/bootstrap/load-plugins/index.js:56:21

- Generator.next

- debuggability.js:313 Promise._execute
[react-jss-bug-306]/[bluebird]/js/release/debuggability.js:313:9

- promise.js:483 Promise._resolveFromExecutor
[react-jss-bug-306]/[bluebird]/js/release/promise.js:483:18

- promise.js:79 new Promise
[react-jss-bug-306]/[bluebird]/js/release/promise.js:79:10

- index.js:96
[react-jss-bug-306]/[gatsby]/dist/bootstrap/load-plugins/index.js:96:17
kof commented 5 years ago

@iamstarkov can you please try this hash based approach on your reproducible code base? I am still benchmarking and thinking how to fix some corner cases, but I am curious if this would fix your problem already, here is an example on how to setup the class names generator https://codesandbox.io/s/oor1k355qq

farcaller commented 5 years ago

@iamstarkov you managed to trip on a very new gatsby bug, huh. I've pushed a workaround to my demo repo.

iamstarkov commented 5 years ago

@farcaller I see

iamstarkov commented 5 years ago

@kof it seems to work with hash-based classNameGenerator

iamstarkov commented 5 years ago

To encounter this bug you need to get different set of JSS based components to be rendered on a server and on a client, (reasons dont matter)

and I think I can manage to describe reproduction scenario:

For example, you have a component:

const triggerStyles = {
  yolo: {
    color: 'red',
    padding: '2em',
    textAlign: 'center',
    backgroundColor: 'white',
  },
};

const Triggered = injectSheet(triggerStyles)(({ classes }) => <div className={classes.yolo}>TRIGGERED</div>);

and then in the app you do:

// App.js

          <div>{typeof window !== 'undefined' ? <Triggered /> : null}</div>

Then following component will most certainly get you shifted counters.

For example we have a Footer after Triggered, let say Footer's counter is N. If we render Triggered only on a server, then on a client Footer's becomes N-1 and N+1 if you negate if statement in your app for Triggered component. Either way, Footer becomes defaced

kof commented 5 years ago

To encounter this bug you need to get different set of JSS based components to be rendered on a server and on a client, (reasons dont matter)

That is supposed to be impossible by react if you mean a rehydration stage. React should give a warning when you do that with or without jss.

HenriBeck commented 5 years ago

If you are rendering a component only on the client and not on the server, then it makes sense that the counter has an offset of 1, but then this wouldn't be a bug of react-jss or jss and more of an implementation problem.

farcaller commented 5 years ago

I don't think that's the case in my scenario – as far as I'm aware both server side and client side are the same (and indeed react warns you if that's not the case)

farcaller commented 5 years ago

Hm. Actually my use case is covered by literally the first entry in material-ui faq. >_(\

iamstarkov commented 5 years ago

@kof

That is supposed to be impossible by react if you mean a rehydration stage.

correct, it should be. yet this is the only reproducible case we managed to come up with persistency

kof commented 5 years ago

@iamstarkov afaik its not a problem of jss, its incorrect assumption that you can rehydrate the client with different render output than the server, which is not supported by react.

So this example is not helpful, since it is not representing a problem we can/should fix.

oliviertassinari commented 5 years ago

@iamstarkov Maybe https://github.com/mui-org/material-ui/issues/13471#issuecomment-434819323 can help.

bstream commented 5 years ago

@kof We tried using the hash-based classname generator, but it does not seem to work for dynamic styles (the dynamic classes all get the same name, and hence the last one specified "wins")

Here is a small test case: https://dist-ivsdauuuly.now.sh/

You can change the value of useHashBasedClassNameGenerator to see both badges get the same style all of a sudden.

image

And here are the styles for the badge (important line at the bottom):

export const styles = theme => {
  const { palette, typography, mixins } = theme;

  return {
    root: {
      ...mixins.basicBoxSizing,
      display: 'inline-block',
      fontSize: '12px',
      fontFamily: typography.primary.fontFamily,
      padding: '2.5px 10px',
      minWidth: '80px',
      lineHeight: '17px',
      color: palette.shades.dark.text.default,
      textAlign: 'center',
      whiteSpace: 'nowrap',
      verticalAlign: 'middle',
      backgroundColor: palette.shades.dark.background.default,
      borderRadius: '3px',

      boxShadow: ({ modifier }) => `0 0 6px 6px ${modifier !== 'success' ? 'tomato' : 'lime'}`,
    },
  };
};
kof commented 5 years ago

yep, hash based with dynamic styles needs to be explored further, its not that simple

HenriBeck commented 5 years ago

I will close this as this isn't a problem of jss.