stitches: Incorrect style order

Bug report

Description

Styles are applied in incorrect order depending on how styled component are composed.

const StyledBase - styled(...)
const Foo = styled(StyledBase, {...}) // works correctly

const Base = (props) => <StyledBase {...props} />;
const Bar = styled(Base, {...}) // StyledBase styles take precedence

Things get even more unpredictable when using Foo & Bar on the same view. Depending on the order of Foo & Bar the styles might or might not apply correctly, see reproduction below.

To Reproduce

  1. Open the following sandbox: https://codesandbox.io/s/nifty-blackburn-m792w?file=/src/App.js
  2. Observe behavior by uncommenting commented out variations one by one. Make sure to refresh the built-in csb browser each time.

Expected behavior

When using styled(Foo, styleObject), rules defined in styleObject should override Foo’s existing styles.

System information

  • OS: MacOS
  • Browser: Chrome, Safari, Firefox
  • Version of Stitches: 0.2+

About this issue

  • Original URL
  • State: closed
  • Created 3 years ago
  • Reactions: 20
  • Comments: 33 (18 by maintainers)

Most upvoted comments

This is being worked on.

This is now fixed in a release candidate under the version 1.2.6-0 under a canary tag. Should be pretty stable if you’d like to upgrade now but expect a final release sometime next week once we finish migrating our products to use the updated version.

Please, don’t hesitate in providing us with any feedback regarding this release. very much appreciated 🙏

Hey @jonathantneal,

Thank you for the clarification. Without knowing much about Stitches internals, I think the assumption that this would work was because a lot of us are coming from styled-components/emotion where it consistently worked that way (and in Stitches < 0.2).

To expand on the use case above, one very common scenario is where we extend UI library components with more app-specific styles:

// App.js
import Button from `@ui/Button`

// Button is a wrapper around StyledButton with extra logic, however StyledButton isn't exported.
const MyAppButton = styled(Button, { ...appButtonStyles })

Where appButtonStyles often contains app state/behavior-specific variants, e.g. isOpen, isActive, etc. Not being able to use variants here basically makes one of the coolest features unavailable.

With all that being said, is there at all a world where this can be supported, even if it’s in future versions?

I am suffering from what I think is the same bug in v1.2.8.

Edit: I experimented a bit, and the CodeSandbox seems to reproduce this with a basic style composition. This seems beyond the immediate situation in this ticket, but I’m wondering if I’m missing someting (?).


Reproduction: https://codesandbox.io/s/determined-almeida-wbizik?file=/src/App.js

I’m not sure if this is 100% reproducible (perhaps it’s flaky?), so a screenshot is included below. I am on Linux Firefox 99.0.1.

image

Thank you so much for addressing this. Deleting all the !importants is going to feel good!

This is now fixed in #875 and is pending release so i’m gonna keep the issue open until we release it publicly

I noticed what I think is the same issue with core: https://codesandbox.io/s/priceless-chatterjee-cpqij

I expected that if the className passed to base({ className }) is a stitches css type, Stitches would Object.assign the two style objects to create a new class.

Fela have a dedicated section on this issue explaining why you have to pass your "rule"s through a util to ensure specificity: https://fela.js.org/docs/latest/advanced/combined-rules#problem-order-matters

Seems to be the same issue?

I raised this previously with @peduarte and he had an API concern regarding the proposed solution for ppl who want to compose multiple class names in the component but I’d personally be okay with always doing the following in that case:

// compose multiple upfront
const text = css(fontSize, color, {});

function Text(props) {
  return (
    <span 
      {...props} 
      className={text({ className: props.className )} 
    />
  );
}

It ensures as much as possible is statically composed at least, since the proposed solution means styles wouldn’t be known until runtime I assume? I’m stumped for other ways this could be solved tho 😬

@jonathantneal If we ordered the CSS the way it was originally created, wouldn’t it still break in cases like this? https://codesandbox.io/s/wonderful-sutherland-jtebo

@frgarciames it’s a known bug and @hadihallak is working on it as we speak 👀

I was pointed here from discord, and just for posterity’s sake I’ll copy over some of my message and the example case I put together. Hopefully more examples are helpful!

I believe there was a change in 0.2.0 with the shift to CSSOM that caused some changes in the ordering of styles.

The general problem is when you have a composition chain of components like StitchesTop > CustomReact > StitchesLow. When StitchesTop and StitchesLow share a variant (let’s imagine it’s color), and StitchesLow has a default variant for color, the default variant can take priority over the variant given to StitchesTop depending on the rendering order of components.

As far as I can tell this is because color is passed to StitchesTop, translated to a classname when passed to CustomReact, which then passes it down to StitchesLow as a classname, but no variant. So it uses the default variant for color. Then it’s just a battle between those two variant classnames based on what order the css was inserted.

I made a demo of this here: https://codesandbox.io/s/jqbgk?file=/src/App.js. If you uncomment the first Box you’ll see that the last line renders correctly as red.

Hi @StephenHaney,

Thanks so much for keeping this on the radar.

Your example is exactly what I would put together. This has proven the most useful in cases where we extend a component with more domain-specific variants like in my example above

Published under 1.2.6 🎉

Oh right, sorry i missed that. Will modify it on Monday so that the same ordering logic run against forward ref components.

@frgarciames Taking a second look here, @hadihallak is correct. This isnt related. Can you open up a new issue about this so we dont add noise to this one? And we can discuss further. Alternatively, join our Discord and post a message in #help and I’ll start a thread there.

Hi everyone, we’re still evaluating this issue — but it may take a rather fundamental change in how Stitches interfaces with CSS classes to make this possible. It likely won’t be a quick fix. So please keep using workarounds for now and we’ll keep you updated.

@tigranpetrossian and anyone that mentioned expecting this to work because you come from SC/Emotion, can you put together a quick example of exactly what you were doing?

Would it look like this?

// Using Styled Components
const Button = styled.button`padding: 10px; color: red;`;

const MyButton = (props) => <Button className={props.className}>{props.children}</Button>;

const Button2 = styled(MyButton)`color: green;`;

Result from SC and the above code when rendering Button2 image

We have similar use-cases and had to come up with weird workarounds in so many places

I wouldn’t say it’s #wontfix.

Yes, it would be difficult for Stitches to reach into Button and know of StyledButton.

However, we might benefit from a new rendering order algorithm that ensures the order you expect. It seems very reasonable.

I would say, we need more time to think about this issue.

Hi @tigranpetrossian,

In the code example, Bar does not extend StyledBase, leaving the injection order up to runtime conditions, when Bar renders another styled component that happens to be StyledBase.

We can resolve this by having Bar extend StyledBase.

const Bar = styled(Base, StyledBase, {...})

Fixed example: https://codesandbox.io/s/recursing-grothendieck-phjij?file=/src/App.js

@hadihallak will this new behavior be available to @stitches/core? https://codesandbox.io/s/x-e6yy7?file=/src/App.js Maybe a cx function like https://emotion.sh/docs/@emotion/css#cx

What about this ? We created a react components library with stitches and our typography (and every component) component has default styles and then, when we try to override these styles with styled function it does not override. This is the example code: https://codesandbox.io/s/stitches-override-example-4yqmo?file=/src/App.js:344-346 Is this behaviour right ? @peduarte

Yes, I think ordering styles based on creation should solve this. I’d be happy to test it out if it’s necessary.

one very common scenario is where we extend UI library components with more app-specific styles

That is fully supported. You should be able to do this.

I was commenting on reaching inside a functional component, which I would not like to do since that would run components out of order (running the component for side effects, then attaching the class names, versus what we do which is pass the class name into the component).

We have been working out another ordering issue, which may happen to do what this issue expects. My first response was connecting these issues, when I should have distinguished these. Let me phrase this another way with some code from the reproduction:

/* StyledBase renders "button". */
const StyledBase = styled("button", {
  display: "inline-flex",
  color: "inherit"
});

/* Base does not extend StyledBase. Base renders StyledBase. */
const Base = (props) => {
  return <StyledBase {...props} />;
};

/* Button1 extends StyledBase. Button1 styles will overwrite StyledBase. */
const Button1 = styled(StyledBase, {
  color: "red"
});

/* Button2 renders Base. It does not extend Base or StyledBase. Its styles are not necessary before/after StyledBase. */
const Button2 = styled(Base, {
  color: "red"
});

With all that being said, is there at all a world where this can be supported, even if it’s in future versions?

Oh yea! I think we can order style injection based on creation. Would this ultimately render what you expect?

/* StyledBase is created first. StyledBase styles will be ordered first. StyledBase renders "button". */
const StyledBase = styled("button", {
  display: "inline-flex",
  color: "inherit"
});

/* Base renders StyledBase. Base does not extend StyledBase. */
const Base = (props) => {
  return <StyledBase {...props} />;
};

/* Button1 is created second. Button1 extends StyledBase. Button1 styles will overwrite StyledBase. */
const Button1 = styled(StyledBase, {
  color: "red"
});

/* Button2 is created third. Button2 renders Base. Button2 styles will overwrite Button1 and StyledBase. */
const Button2 = styled(Base, {
  color: "red"
});

I originally thought this issue might be related to our “order by creation” issue, but then I saw, based on the reproduction, that it was more a side-effect.

Perhaps we should reopen this. I bet the team would be in favor! I feel I was a little stuffy in my last response, anyway (sorry)!

I’d like your thoughts, but the new presumption would be that we want the CSS to be ordered the way it was originally created.