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

Unable to get desired style precedence without TssCacheProvider #115

Closed
ryancogswell opened this issue Sep 27, 2022 · 20 comments
Closed

Unable to get desired style precedence without TssCacheProvider #115

ryancogswell opened this issue Sep 27, 2022 · 20 comments

Comments

@ryancogswell
Copy link

I was attempting to upgrade from v3 to v4, but had to revert it after being unable to achieve style ordering similar to v3 without the TssCacheProvider. For my v3 setup, I create a prepend cache for MUI and a non-prepend cache for tss-react. This ensures that the styles generated by makeStyles always win over MUI styles when CSS precedence is otherwise the same (something that was also true when using JSS's makeStyles though the mechanism for controlling the relative order was different). In the migration guide, I saw the example using createMakeAndWithStyles for this purpose, however that doesn't give me any way to vary the tss-react cache using context. This is important for me since I actually have two MUI caches and two tss-react caches -- one pair for ltr and one pair for rtl. These caches get swapped dynamically as the user changes the language of the page.

Below are the relevant pieces for how I'm using v3:

const muiCacheLtr = createCache({
   key: "muiltr",
   prepend: true
});
const tssCacheLtr = createCache({
   key: "tssltr"
});

const muiCacheRtl = createCache({
   key: "muirtl",
   prepend: true,
   stylisPlugins: [prefixer, rtlPlugin]
});
const tssCacheRtl = createCache({
   key: "tssrtl",
   stylisPlugins: [prefixer, rtlPlugin]
});
// Then later inside of App:
   return (
      <CacheProvider value={theme.direction === "rtl" ? muiCacheRtl : muiCacheLtr}>
         <TssCacheProvider value={theme.direction === "rtl" ? tssCacheRtl : tssCacheLtr}>
            <ThemeProvider theme={theme}>

Any recommendation on how to do this in v4?

@garronej
Copy link
Owner

Hi @ryancogswell how are you doing?
Thanks for reporting.
I released v4 too soon. I'm verry sorry about that.
A big PR on MUI that should fix all your issues has just been merged: mui/material-ui#34451

Just have to wait the next MUI release and everything should be fixed.

Best regards,

@ryancogswell
Copy link
Author

Hi @garronej,
I'm doing fine. Thanks for the really quick response. I'm having a difficult time seeing how that PR will address my issue, but I'll wait till after the next MUI release and then see if things are different.
Thanks,
Ryan

@ryancogswell
Copy link
Author

ryancogswell commented Sep 27, 2022

I received the notification of the MUI release immediately after my last comment, so I was able to immediately try it out. I am still seeing my issue, but it may be more specific than what I first realized. The styles that I was noticing were with a usage of Grid and media-query-specific styles from MUI were winning over styles from makeStyles.

It turns out I can also reproduce this without tss-react in play at all. The same issue occurs using the sx prop:

import * as React from "react";
import { styled } from "@mui/material/styles";
import Box from "@mui/material/Box";
import Paper from "@mui/material/Paper";
import Grid from "@mui/material/Grid";

const Item = styled(Paper)(({ theme }) => ({
  padding: theme.spacing(1),
  textAlign: "center"
}));

export default function BasicGrid() {
  return (
    <Box sx={{ flexGrow: 1 }}>
      <Grid container spacing={2}>
        <Grid item xs={12} sx={{ maxWidth: 300 }}>
          <Item>xs=12, max-width: 300</Item>
        </Grid>
        <Grid item xs={12} sx={{ "&&": { maxWidth: 300 } }}>
          <Item>xs=12, higher precendence max-width: 300</Item>
        </Grid>
      </Grid>
    </Box>
  );
}

Edit media query style precedence

In the sandbox above, the second grid item displays the way I would want the first grid item to display (with the max-width of 300 winning). With the separate tss-react cache in v3, the styles from makeStyles win over media-query-specific styles from MUI, but in v4 (without the TssCacheProvider) they do not.

@garronej
Copy link
Owner

Oh no!
I think it's related to this argument that I lost 😢

See my patched cx in this repo: https://stackblitz.com/edit/tss-react-frmsvm?file=index.tsx
Can you confirm it's related to your problem?

@ryancogswell
Copy link
Author

Yes, I think that is the problem I'm seeing -- media query style rules from MUI are being injected after non-media query style rules from tss-react.

I could restore the old behavior without you needing to reintroduce TssCacheProvider if createMakeAndWithStyles accepted either EmotionCache or a callback (() => EmotionCache) for the cache parameter.

@garronej
Copy link
Owner

Ok thank you for clarifying this for me.

I could restore the old behavior without you needing to reintroduce TssCacheProvider if createMakeAndWithStyles accepted either EmotionCache or a callback (() => EmotionCache) for the cache parameter.

It does already :)

https://docs.tss-react.dev/update-to-v4#less-than-tsscacheprovider-greater-than-removedimage

https://docs.tss-react.dev/cache
image

@garronej
Copy link
Owner

It turns out I can also reproduce this without tss-react in play at all. The same issue occurs using the sx prop:

Well, this is good new for us, it will maybe help me convince @emotion and MUI that it's an issue worth addressing.
I knew it could happen but I didn't have an actual case where it happens.

Would you open an issue about it in the MUI repo and tag me?
If you don't have time don't worry, you already did a lot for TSS.

Best regards,

@garronej
Copy link
Owner

What I am wondering is if this is happening only to some pepole or if everyone is experiencing this kind of issues but don't bother opening an issue about it.

@ryancogswell
Copy link
Author

It does already :)

It doesn't support passing in a callback for the cache parameter.

Well, this is good new for us, it will maybe help me convince @emotion and MUI that it's an issue worth addressing.

I don't think this case changes the overall arguments in that thread. Changing the behavior in @emotion now would have too much impact on users dependent on the current behavior.

@garronej
Copy link
Owner

garronej commented Sep 27, 2022

It doesn't support passing in a callback for the cache parameter.

Ok sorry I didn't read carefully enough. I am releasing an beta (4.2.0-beta.1) that implement your proposal.
Let me know if it fits your need (see my mention in the comment).
#116

I don't think this case changes the overall arguments in that thread. Changing the behavior in @emotion now would have > too much impact on users dependent on the current behavior.

Thank you for taking the time to read the thread.
I guess you are right. I was hoping for an option that would let the user decide if media quarry should increase specificity.
I think it should be disabled by default in @emotion but enabled in MUI.

I worked a lot to make it possible that TSS shares the same cache with MUI, this was the whole point of V4. But because of this problem with media query I'm afraid this switch will cause more harm than good.

What is your opinion on this?
Do you think the official recommended way of setting up TSS should be without providing custom cache (it makes it easier to configure SSR since there is no need for specific instruction) or should I recommend that people provide a cache so it works everywhere as expected with no edge case?

@ryancogswell
Copy link
Author

What is your opinion on this?

The application I work on doesn't use SSR (and I don't expect it to anytime in the next few years if ever), so I don't have an appreciation for the complexities the separate cache introduces for SSR. When troubleshooting styles in the browser developer tools, I prefer the effect of the separate cache where the styles from our customizations are completely separated from the MUI styles.

I suspect for people starting from scratch, the edge cases are less of an issue because they will do what they need to do to get customizations to work and there aren't a lot of different MUI components with media query rules in the library's styles. The edge cases are a bigger issue for people migrating a large code base from JSS with MUI v4 to tss-react with MUI v5 where there isn't an easy way to find the places that may have issues (aside from visually checking everything where it would be easy to miss things).

As far as the "official recommended" setup, probably go with the simpler route (no custom cache), but provide examples of both with a brief explanation of the potential reasons for using a custom cache for tss-react.

I am releasing an beta (4.2.0-beta.1) that implement your proposal.
Let me know if it fits your need (see my mention in the comment).

I'll try it out sometime in the next week or two. There isn't any urgency for me to upgrade tss-react to v4, I was just upgrading some packages that were going to force regression testing a lot of our app, so I decided to get other packages up-to-date at the same time.

@garronej
Copy link
Owner

@ryancogswell thank you very much for taking the time to give me your insight on this.
I think I will follow up with your suggestion and write a documentation page for troubleshooting peepole trying to migrate large codebase to MUI v5.

I leave the patch I published for you in beta until you confirm it fits your needs.

Best regards,

@Semigradsky
Copy link
Contributor

Was TssCacheProvider a problem, why was it removed?
In my app some parts can be written by different teams with different UI Kits versions, so I need to parametrize cache keys, smth like this:

const ThemeProvider: FC<ThemeProviderProps> = ({
  theme,
  muiCacheKey = 'mui',
  tssCacheKey = 'tss',
  children,
}) => {
  return (
    <CacheProvider value={getMuiCache(muiCacheKey)}>
      <TssCacheProvider value={getTssCache(tssCacheKey)}>
        <MuiThemeProvider theme={theme}>
          {children}
        </MuiThemeProvider>
      </TssCacheProvider>
    </CacheProvider>
  )
}

I am not sure how I can pass tssCacheKey to createMakeAndWithStyles without a provider. Looks like I need to write my own TssCacheProvider.

@garronej
Copy link
Owner

garronej commented Sep 30, 2022

Was TssCacheProvider a problem, why was it removed?

I guess I can reintroduce it. I though it make more cense to provide the cache as a parameter when creating makeStyles but I see there are some usecase for having a provider.

EDITl: Now I remember why I removed it, it's because it forces tss-react to be a peer dependency when it's used in a library. Are you sure you can't make it work without a provider?

EDIT2: I guess I can document the fact that if you use tss-react in a library, don't, or document that tss-react is a peer dependency of your module.

@Semigradsky
Copy link
Contributor

I think if you use library X in your package, this library X should be in dependencies or peerDependencies.

@garronej
Copy link
Owner

Yes of course but I mean, for example mui-datatables is using tss-react internally, they don't want to have in their install instruction yarn add mui-datatables tss-react.
Users don't have to know about tss-react being used internally.

@Semigradsky
Copy link
Contributor

Ok, I understand now.

@garronej
Copy link
Owner

I'm going to restore the provider

garronej added a commit that referenced this issue Sep 30, 2022
@garronej
Copy link
Owner

@Semigradsky @ryancogswell I reintroduced <TssCacheProvider /> in 4.2.0:

https://docs.tss-react.dev/cache#use-a-specific-provider

@garronej
Copy link
Owner

@ryancogswell I did as you suggested https://docs.tss-react.dev/troubleshoot-migration-to-muiv5-with-tss

I'll close this issue now.

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