makeStyles (v8): nondeterministic style order when theme changes

[jtbandes]

Environment Information

• Package version(s): 8.39.0
• Browser and OS versions: macOS Safari and Chrome (probably all browsers)

Please provide a reproduction of the bug in a codepen:

https://codesandbox.io/s/lucid-gauss-0cspe?file=/src/index.tsx

const useStyles = makeStyles((theme) => ({
  root: {
    fontWeight: "normal",
    color: theme.palette.themePrimary
  },
  bold: {
    fontWeight: "bold"
  }
}));

...

<div className={`${styles.root} ${styles.bold}`}>Am I bold?</div>

Actual behavior:

Changing the theme color makes the "Am I bold?" text no longer bold:

When changing the theme color dynamically, style rules which depended on the theme are apparently re-inserted at the end of the document, meaning their precedence can change relative to existing rules that didn't change. In this example, although only the color is changing, the effective fontWeight also changes because the bold: rules now appear earlier in the source than the root: rules.

Expected behavior:

"Am I bold?" should always be bold, because bold comes after root in the makeStyles call.

Priorities and help requested:

Are you willing to submit a PR to fix? Maybe, if I can get guidance for where to make the fixes and how to test them

Requested priority: N/A

Products/sites affected: N/A

[khmakoto]

@jtbandes thank you for filing this. I can confirm the repro.

@microsoft/cxe-red are we still supporting the makeStyles version that is inside of v8?

[ecraig12345]

It seems like a major oversight that any version of make-styles is being exported from v8. Unless we actually intend to keep it in alignment with the new make-styles, that just seems like all kinds of problems waiting to happen. Unfortunately I don't know if we can remove it at this point (technically that would be a breaking change) but we should certainly at least deprecate it with a message about what to do instead.

(I think the way it ended up being included was because it was copied along with the ThemeProvider code when we got rid of @fluentui/react-theme-provider. I think ThemeProvider is intended to be supported for v8, even though we ultimately decided to go a different direction, but supporting the old version of makeStyles makes less sense.)

[jtbandes]

are we still supporting the makeStyles version that is inside of v8?

Does this comment mean makeStyles is an outdated API? Do you have recommendations for what other APIs we should be using instead of the v8 makeStyles (as an external non-MS project)? Where is the latest documentation on this?

[ecraig12345]

are we still supporting the makeStyles version that is inside of v8?

Does this comment mean makeStyles is an outdated API? Do you have recommendations for what other APIs we should be using instead of the v8 makeStyles (as an external non-MS project)?

The version of the API being exported from @fluentui/react is an outdated experimental version which is not intended for continued support. The new version is in @fluentui/make-styles and @fluentui/react-make-styles which are in beta, and I'm not sure if/how they're intended to work with version 8. You can read more about the new APIs under https://aka.ms/fluentui-storybook => "Developer" section.

[jtbandes]

OK, thanks for the info. What does it mean that the @fluentui/react version was "experimental"? Is it specifically the makeStyles API that's not getting continued support, or the entirety of @fluentui/react?

By the way, I tried translating this example to 9.0.0-beta and the problem seems to be fixed: https://codesandbox.io/s/festive-carlos-r0tiw?file=/src/index.tsx However I'm not sure whether it was intentionally fixed or whether reproducing the problem requires a different setup with v9.

[khmakoto]

OK, thanks for the info. What does it mean that the @fluentui/react version was "experimental"? Is it specifically the makeStyles API that's not getting continued support, or the entirety of @fluentui/react?

It means the @fluentui/react v8 version of makeStyles is experimental, not the entirety of the library.

By the way, I tried translating this example to 9.0.0-beta and the problem seems to be fixed: https://codesandbox.io/s/festive-carlos-r0tiw?file=/src/index.tsx However I'm not sure whether it was intentionally fixed or whether reproducing the problem requires a different setup with v9.

I think the version in v9 is intentionally intended to always apply last the class names that are applied later.

[jtbandes]

It means the @fluentui/react v8 version of makeStyles is experimental, not the entirety of the library.

Ah, whew 😅 Was there a note about this somewhere in the docs? I don't see anything about it the .d.ts header comments.

Also, is there any info about the timeline of when 9.0.0 is going to graduate out of beta? I'm wondering if there are plans to write a more thorough migration guide from v8 — since we are using functions like createTheme(), registerIcons(), components like ColorPicker, MessageBar, and other things that don't seem to be mentioned at all in the Migrating page right now.

Sorry for abusing the comment thread of this ticket, but it's really helpful to hear directly from engineers working on the project rather than just bumbling around the maze of different (and sometimes outdated) documentation pages ourselves! 💟

[ecraig12345]

Also, is there any info about the timeline of when 9.0.0 is going to graduate out of beta? I'm wondering if there are plans to write a more thorough migration guide from v8 — since we are using functions like createTheme(), registerIcons(), components like ColorPicker, MessageBar, and other things that don't seem to be mentioned at all in the Migrating page right now.

We're hoping to have a RC version sometime in December I think. Not sure what the timeline is for final release, but as we get closer to that and beyond, there will definitely be more extensive documentation/migration info added.

Caveat with the initial release is that it won't include the full set of components from v8. We plan to get there eventually, it will just take some time. I know ColorPicker in particular is lower priority, and not sure about MessageBar (we've initially been prioritizing components based on needs of internal partner teams). So one of the things we'll need to document is best practices for handling this intermediate state with a mix of v8/v9 components.

For icons, in v9 we're switching to SVG icons from @fluentui/react-icons (which also uses the standard MIT license without additional restrictions), so registerIcons() will no longer be needed. We're currently working out a few issues with that package, so I'd recommend waiting to use it until version 2.x is released, hopefully in the next couple weeks.

[gouttierre]

@ecraig12345 - Do you know if we moved the SVG icons from @fluentui/react-icons? If so, can we close out this item.

[ecraig12345]

The icons question is not related to the original issue, so doesn't really affect whether the issue should stay open or not.

For the original question about the v8 makeStyles, @khmakoto and I agreed that it shouldn't have been exported from v8 since it wasn't ready for partner use and is being replaced in v9 with a completely different implementation. To avoid a breaking change in case anybody had mistakenly started using it, we deprecated it in #20712 instead of removing. So I think it makes sense to close this as "won't fix."

[ecraig12345]

Oh also we changed the package names for the new version of makeStyles to @griffel/core and @griffel/react. Just FYI in case it's relevant to anyone reading this later.

[jtbandes]

Thanks for the pointer to Griffel. But it looks like that version of makeStyles doesn't support a theme object/provider?

[khmakoto]

@jtbandes the styling system has been uncoupled from the theming infrastructure so they are more free to be used alongside other solutions, but everything should still work by importing the tokens object from @fluentui/react-theme:

import { makeStyles } from `@griffel/react`;
import { tokens } from '@fluentui/react-theme';

const useStyles = makeStyles({
   div: {
      color: tokens.colorNeutralForeground1
   }
})

And then in your app:

import { webLightTheme, FluentProvider } from '@fluentui/react-theme';
import { useStyles } from './useStyles'; // The function above

const Example = () => {
   const styles = useStyles();

   return (
      <FluentProvider theme={webLightTheme}>
         <div className={styles.div}>This is a div with colors coming from the theme</div>
      </FluentProvider>
   );
};

[jtbandes]

Thanks. Is there documentation on this anywhere?

Is it true that the token-based approach does not allow computing values inside of makeStyles based on values from the theme, just referencing pre-defined values from the theme?

[khmakoto]

@jtbandes we're working on improving documentation right now, but you might find some here.

It is not allowed inside a makeStyles function call but you can still do it by referencing the useStyles hook that makeStyles returns inside of another hook/functional component that has access to the state of your object to compute the correct styles. The reason for this is to be able to do build style optimizations that require no computing parts in the makeStyles call. You can take a look at this in our own components.

A really simple example of this would be something like:

import { makeStyles, mergeClasses } from `@griffel/react`;
import { tokens } from '@fluentui/react-theme';

const useStyles = makeStyles({
   base: {
      color: tokens.colorNeutralForeground1
   },
   // Want to change color when the active prop has been passed in
   active: {
      color: tokens.colorBrandForeground1
   }
});

const useComponentStyles = (state: { active: boolean, className: string }) => {
   const styles = useStyles();
   state.className = mergeClasses(
      styles.base,
      // You only pass that className when active prop/state is present
      state.active && styles.active,
      state.className
   );
};

One last thing, using mergeClasses ensures the styles are applied in the order you provided them, removing any collisions.