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

Are there plans to support dynamic parameters #90

Closed
itmanyong opened this issue Apr 7, 2022 · 6 comments
Closed

Are there plans to support dynamic parameters #90

itmanyong opened this issue Apr 7, 2022 · 6 comments

Comments

@itmanyong
Copy link

Creating styles that are more real-time can feel incongruous! It is not known if Griffel has any plans to support dynamic parameters. Similar to:
`
const useClasses= makeStyles((color)=>({fontColor:{color}}))

const classes = useClasses('red')
`

@layershifter
Copy link
Member

layershifter commented Apr 11, 2022

Hey, there is no plans to do this currently or near future. It's our intentional decision. Let me explain it a bit.

makeStyles({
  root: { fontColor: "red" }
});
// => .hash { font-color: red }
  • Result there is stable
  • Does not depend on user's input
  • Can be safely executed in any environment (browsers, servers, Node)
// ⚠️ not real code
makeStyles(color => {
  fontColor: color;
});
// => .??hash?? { font-color: ??? }
  • Result there is not stable and may vary based on users input

  • Option 1: precompute all input combinations (no go)
  • Option 2: use CSS variables:
    color => {
      fontColor: color;
    };
    // => .hash-var { font-color: var(--hash) }
    • The open question is how to apply that CSS variable then 💥

Another issue with dynamic params that nothing prevents following:

// ⚠️ not real code, not something that we want to support
const useClasses = makeStyles(isFoo => ({
  color: "red",
  ...(isFoo & { color: "blue" })
}));

In this case static evaluation will be impossible at all.


Our current solution for these cases is to use inline styles or combination with CSS variables:

function App(props) {
  return <div style={{ color: props.color }} />;
}
const useClasses = makeStyles({
  root: { color: 'var(--color)',
})

function App(props) {
  const classes = useClasses()

  return <div className={classes.root} style={{  '--color': props.color }} />
}

P.S. We can reconsider this later based on consumers feedback, but currently we don't have enough data to turn it into usable API.

@heho
Copy link

heho commented May 31, 2022

@layershifter I do not think that this is a solution that is maintainable in the long run. I tested this on a small scale project and it was wonky at best. On one hand, it requires a lot of additional work from the user which can be faulty and can lead to mixing style with logic. On the other hand it is not possible to typecheck which makes early detection of mistakes during development even harder.

If I could choose an API, it would probably look something like this:

const useClasses = makeStyles((props) => {
  root: { color: props.color }
}, { color: "#aabbcc" }) // type of props is the same as the second parameter

function App(props) {
  const classes = useClasses({}, { color: "red" }) // same second argument types as props

  return <div className={classes.root} />
}

Then the makeStyles function injects some hashes into the parameters and the useClasses function handles provision of the variables via those hashes. This effectively reduces the amount of work needed to be done for an update while still allowing for changing props to influence the style. However, the work for the creation and injection of the hashes could be a bit more involved, since it would be useful to be able to pass deep objects. But since this work can be done on load it might not be as bad.
There is also the possibility to encourage the current API whilst providing an additional API like the one mentioned for the cases where it is needed.

@layershifter
Copy link
Member

Then the makeStyles function injects some hashes into the parameters and the useClasses function handles provision of the variables via those hashes. This effectively reduces the amount of work needed to be done for an update while still allowing for changing props to influence the style. However, the work for the creation and injection of the hashes could be a bit more involved, since it would be useful to be able to pass deep objects. But since this work can be done on load it might not be as bad.
There is also the possibility to encourage the current API whilst providing an additional API like the one mentioned for the cases where it is needed.

There are two problems with that, read below.

CSS variables generation

To support SSR and build time optimisations we need a deterministic way to generate CSS variables in Browser & Node environments to avoid collisions:

const useClasses = makeStyles((props) => ({
  root: { color: variables.color }
}))
  • if it will be just --color i.e. .rule { color: var(--color) } it can unpredictably collide with other variables/scopes
  • it will be a random hash i.e. (Math.random() + 1).toString(36).substring(7) it will cause problems for SSR/build time processing as these hashes will be different from build to build
  • I was trying to prototype a solution around stack traces to get filename into hash, but there are no good results

To summarise: currently I don't know a predictable way to generate CSS variables that would scale.

API changes

Well, let's assume that we can generate CSS variables, but how they will be passed?

const useClasses = makeStyles((props) => ({
  root: { color: variables.color }
}))

function Foo() {
  // Where to pass variables?
  const classes = useClasses({ color: 'foo' })
}

Something like that could work:

function Foo() {
  // Where to pass variables?
  const [classes, styles] = useClasses({ color: 'foo' })

  return <div className={classes.root} style={styles} />
}

But I don't see this intuitive and scaling to other JS frameworks/libraries rather than React.


I prototyped a solution that should be typesafe: https://codesandbox.io/s/clever-sunset-5ju2bq?file=/src/App.tsx
But until CSS variables generation will be solved I don't think that it something that could go to the core.

@heho
Copy link

heho commented Jun 1, 2022

Looking at the simpler Problem first, does anything speak against injecting in into a style bucket sheet with sheet.insertRule(":root{--<hash>: <value>}"); then obtaining a reference to that style rule and modifying the rule.style.cssText on prop change? Then you would not need to pass any styles around by hand.

It seems to me that the hash could be generated from the encoded style object by injecting placeholder into the variables. However, in corner cases this would create collisions.

@layershifter
Copy link
Member

Looking at the simpler Problem first, does anything speak against injecting in into a style bucket sheet with sheet.insertRule(":root{--<hash>: <value>}"); then obtaining a reference to that style rule and modifying the rule.style.cssText on prop change? Then you would not need to pass any styles around by hand.

Well, if it's global then it brings few more interesting questions:

  • how to do CSS extraction later (it's impossible, this feature is only runtime thing)

  • I still should be able to do:

    function Foo() {
      const classes1 = useClasses({ color: 'red' })
      const classes2 = useClasses({ color: 'blue })
    
      return (
         <>
           <div className={classes1.root} />
           <div className={classes2.root} />
         </>
      )
    }

    Then it's really questionable what will "hash" in this case 🤯

    👆 It's also show that this API does not scale well as I may have 10 divs. Do I need to call useClasses() for each of them?

@heho
Copy link

heho commented Jun 17, 2022

sorry for coming back to you so late. Corona got me.

Now assuming there is a good solution for creating a hash one could create a helper class with each call of useClasses that is passed around with the classes. This helper class then gets all the variables for the instance of useClasses.

function Foo() {
  const classes1 = useClasses({ color: 'red' }) // produces something like { root: ["abc123", "h3lp3r"], helpers: ["h3lp3r"] }
  const classes2 = useClasses({ color: 'blue })  // produces something like { root: ["abc123", "r3pl3h"], helpers: ["r3pl3h"] }

  return (
     <>
       <div className={classes1.root} />
       <div className={classes2.root} />
     </>
  )
}

where the previous still holds true that the helper classes get updated when the props object is updated. However this looks like it would make the problem of finding hashes even worse.

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

3 participants