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

fragment tag for composition #244

Open
satya164 opened this issue Oct 10, 2018 · 33 comments
Open

fragment tag for composition #244

satya164 opened this issue Oct 10, 2018 · 33 comments
Assignees
Labels
core team 🧬 Issues reserved for the core team feature: proposal 💬 New feature proposal that needs to be discussed

Comments

@satya164
Copy link
Member

satya164 commented Oct 10, 2018

Problem

Currently it's not possible to compose styles from multiple classnames together. Previously we had a include helper which would take a class name and return the unprocessed CSS text. I removed it because it wasn't strightforward to implement it with the new approach and I haven't needed to use it personally. In addition, an extra class name was generated whether you used it anywhere or not.

Another usecase is that once you get into an interpolation inside the tagged template, we don't parse the nested interpolations. So it's not possible to do something like following:

const Box = styled.div`
  font-size: 14px;

  ${borders
    .map(({ p, w }) => `border-${p}: ${w}px solid ${props => props.color}`)
    .join('\n')};
`;

styled-components has exports css tag to support this. However, the css tag is used for class names in Linaria, so we can't do the same.

Prior art

Emotion allows you to interpolate a class name which inlines its styles: https://emotion.sh/docs/composition

One problem with this approach is that you're interpolating a class name, but getting the CSS text instead. This breaks the following use case where you might want to refer to another class name:

const paragraph = css`
  font-size: 14px;
`;

const article = css`
  backgrounc-color: white;

  .${paragraph} {
    font-size: 16px;
  }
`;

Current solution

Currently, you could do something like this:

const shared = `
  font-size: 18px;
`;

const Title = styled.h1`
  ${shared};

  color: black;
`;

There are 2 issues with this approach:

  1. There's no syntax highlighting or autocomplete for the CSS
  2. It's not possibe to use dynamic interpolations like you would in styled

Proposal

We could add a new fragment tag to allow this use case:

const shared = fragment.css`
  font-size: 18px;
`;

const Title = styled.h1`
  ${shared};

  color: black;
`;

The fragment tag itself won't generate any class names, but when the result is interpolated, it will inline the CSS text along with the dynamic interpolations. It'll work very similarly to how styled-components handles interpolation of css tagged template literals. As for parsing, it would be parsed similar to how the styled.x tags are parsed.

Note: Initially I thought of having just fragment, but went for fragment.css since it allows us to have syntax highlighting and autocomplete.

Since the fragment tag won't generate a class name, to use it as a class, you could do something like this:

const title = css`${shared}`;

You could even inline it if you don't like creating an extra variable:

<h1 class={css`${shared}`} />

Why not add it

It requires extra code, especially if we want to support interpolations and don't want to leave the CSS code for the fragment inside the JS bundle.

@silvenon
Copy link
Contributor

Just to check, this route is impossible/unacceptable?

const card = css`
  background: #fff;
  box-shadow:
    0 1px 3px rgba(0, 0, 0, 0.12),
    0 1px 2px rgba(0, 0, 0, 0.24);
`
const article = css`
  font-size: 18px;
  ${card};
`

console.log(card)
// "a123"
console.log(article)
// "b123 a123"
// ☝️it's not a mixin, it just concatenates class names

@satya164
Copy link
Member Author

@silvenon we cannot do that because then the following will not be correct:

const paragraph = css`
  font-size: 14px;
`;

const article = css`
  background-color: white;

  .${paragraph} {
    font-size: 16px;
  }
`;

Here the desired result was to use different styles for paragraph when nested inside article, but concatenating the class names will mean that all the styles from paragraph will also be applied to article.

You can instead do following:

const card = css`
  background: #fff;
  box-shadow:
    0 1px 3px rgba(0, 0, 0, 0.12),
    0 1px 2px rgba(0, 0, 0, 0.24);
`;

const article = css`
  font-size: 18px;
`;

<div class={cx(card, article)} />

@silvenon
Copy link
Contributor

I thought that it would be possible to detect .${paragraph} { vs ${paragraph}; and act accordingly. Yes, we can concatenate classes with cx, but this way would be much shorter, especially with styled. Also, unlike fragments, concatenating class names wouldn't duplicate styles.

But if that's not possible, fragment API (mixins?) sounds good! Although instead of adjusting API to text editor plugins, we can adjust text editor plugins to API instead? What I mean is that it might be better to use fragment instead of fragment.css (I'd be the first one to submit a PR to styled-components extension for VS Code).

@satya164
Copy link
Member Author

I thought that it would be possible to detect .${paragraph} { vs ${paragraph}; and act accordingly

It could be possible if we parse the CSS as it's constructed. But then we also have to distinguish between class names being interpolated and plain strings being interpolated. It adds complexity which isn't worth it imo. It's also not straightforward to understand.

Yes, we can concatenate classes with cx, but this way would be much shorter, especially with styled. Also, unlike fragments, concatenating class names wouldn't duplicate styles.

I think the main appeal of composition in emotion is that when you compose 2 classes, you don't have to worry about the cascade, unlike just concatenating 2 class names. In emotion, it'll duplicate the styles to acheive the correct styles, which is what fragment is aiming to do. Also if we make the feature look likewhat emotion does, but just concatenate class names, it might be confusing too. So I think better to be explicit and use cx.

Although instead of adjusting API to text editor plugins, we can adjust text editor plugins to API instead

That'd certainly be preferable, but I highly doubt that they'll accept such a PR to styled-components extension, since styled-components doesn't support such a feature. AFAIK they rejected a PR adding support for css prop which emotion has, because styled-components doesn't support it.

Another problem is that fragment is a very generic name, so it'll be impossible to convince them to highlight it as CSS. For example, another library might export something named fragment, in which case, we wouldn't want to parse that.

@otbe
Copy link

otbe commented Feb 14, 2019

Thats really the last missing piece to switch to linaria. I have 2 projects in my mind. In one project we use css modules (with compose) and in the other project we use styled-components. And we do things like:

// generic mixin for margins, especially nice when using TS
const margins = (marginProps) => `margin: ${compute(marginProps)}`;

const Wrapper = styled.div`
  ${margins};
  background-color: green;
`

If I get you correct, this should be possible with fragment.css?
Would love to see somthing like this :)

@ferdaber
Copy link

ferdaber commented Feb 16, 2019

What if another function is exported from the library that where you can call it that allows you to detect that the intent is to compose?

import { cx, css, compose } from 'linaria'

const paragraph = css`
  font-size: 14px;
`;

// this composes the other class's styles
const paragraphArticle = css`
  background-color: white;

  ${compose(paragraph)};
`;

// this selects the other class
const article = css`
  background-color: white;

  .${paragraph} {
    font-size: 16px;
  }
`;

@satya164
Copy link
Member Author

@ferdaber We did exactly the same previously. but it's not very useful because it's not possible to use interpolations etc. and it just becomes a fancier string interpolation. It's also not straightforward to implement.

@ferdaber
Copy link

I think the issue with fragment is that it may not be obvious to a user importing it that it's a fragment without looking at the source code. I know with my team we have a lot of "mixins" but some are actually fragments but some are nearly-full styles of components that need to be composed, and it might be a bit of mental overhead to consider when to actually do className={css(myFragment)} vs just className={myFragment}.

Can you elaborate why this method would make it impossible to use interpolations? I think this is an awesome library but this missing feature is something I use very, very often in emotion currently.

@satya164
Copy link
Member Author

Can you elaborate why this method would make it impossible to use interpolations

Because we can't support dynamic interpolations in the css tag, and will throw a syntax error if you try to use it. This alone makes the use case very limited. We could make it so that only css tag can be composed inside another css tag and only styled tag can be composed inside another styled tag, but it'll be better to have a solution which is interoperable.

it may not be obvious to a user importing it that it's a fragment without looking at the source code

It's a valid argument, but it's fair to assume that if you are importing something, you need to know what it is that you're importing. It's the same in other parts of JS as well.

It comes down to how you organize the code. You can keep fragments in a separate file (like people usually do for sass mixins etc.) to make it super obvious.

when to actually do className={css(myFragment)} vs just className={myFragment}

You should avoid doing className={css(myFragment)} because unlike a runtime solution like emotion, in linaria, this means every time you do that, it'll create a new CSS rule which is basically duplicated code.

@ferdaber
Copy link

ferdaber commented Feb 16, 2019

Ah gotcha, I thought you were talking about any interpolation in general, because I think that css still supports partial evaluation of variables, right? css'${colors.white}'

With css(myFragment) I was referring to your proposal, wouldn't they be equivalent?

<div className={css`${myFragment}`} />
<div className={css(myFragment)} />

@brandonkal
Copy link

brandonkal commented May 29, 2019

What if array destruturing was used?

Normally:

const [paragraph] = css`
  color: black;
`

Then to grab the rules for composition within another css or styled tag:

const [paragraph, pRules] = css`
    color: black;
`

const main = styled.main`
  .${paragraph} {
       color: green;
   }

.copy {
  ${pRules}
}
`

@sshmyg
Copy link

sshmyg commented Jun 4, 2019

Will 1.4v get some solution for this issue?

@trusktr
Copy link

trusktr commented Oct 21, 2019

Emotion allows you to interpolate a class name which inlines its styles:

Unfortunately Emotion devs are removing that feature. The console will throw a red error at you, telling you it that the the feature will be removed in an upcoming version.

@jayu jayu added feature: proposal 💬 New feature proposal that needs to be discussed and removed rfc labels Apr 1, 2020
@jayu
Copy link
Contributor

jayu commented Apr 4, 2020

Styles composition would be extremely useful once we introduce react-native support #236
We could potentially share more code between react and react-native

@jayu jayu added the core team 🧬 Issues reserved for the core team label Apr 4, 2020
@jonatansberg
Copy link

We just ran in to a case where we would really benefit from this. A colleague of mine is currently trying to write a codemod for migrating ~50 separate apps (roughly 10 000 components) from Emotion 9 to Linaria, and this is a pattern thats used in a lot of them.

Would it be possible to keep the current behaviour of css-tags serialising to classNames (e.g. having .toString return a class name string) and then enable css-tag composition by either:

a) overriding the .toJSON method to return a css-string or object litteral, or

b) use the same kind of "sniffing" thats implemented here to get the rules for css-tags and the class name for styled components?

@jayu
Copy link
Contributor

jayu commented May 9, 2020

@jonatansberg your case is dope! Can you post which exact syntax you want to have? Because there are various limitations while implementing this, I was thinking a lot recently about how to approach this, I want to achieve better code reusing.
Btw if you manage to write this codemod it would be awesome to open-source it :D Nested css interpolations would be hard to implement, I was thinking about being able to write this

const shared = css``

const className = css`
${shared}

`

And I would try implement 'sniffing' based on the prepending dot. It would allow to use css as className or interpolate it's styles into other css or styled.

@jonatansberg
Copy link

Yeah, that is pretty much what I'm after as well. I understand that this is a pattern that has a lot of caveats, and that you're most of the time better of solving this in other ways. Unfortunately since this used to work with Emotion, we need to find a way to either codemod it away or make it work with Linaria...

Here is a complete example. Code like this:

const shared = css` 
  color: peachpuff;
`;

const otherStyles = css`
  ${shared}
  background: crimson;
`

const InlineComponent = styled.span`
  text-transform: uppercase;
`

const Component = styled.div`
  padding: 1rem;
  ${otherStyles}

  ${InlineComponent} {
    font-weight: bold;
  }
`

Should preferably turn in to something thats the equivalent to this:

.InlineComponent {
  text-transform: uppercase;
}
.Component {
  padding: 1rem;
  color: peachpuff;
  background: crimson;
}
.Component .InlineComponent {
  font-weight: bold;
}

This works just fine when you remove the css tags and just use string literals, but thats really hard to do in an automated way since the css tags could be used either as a class name or as a style string...

@jayu
Copy link
Contributor

jayu commented May 9, 2020

Good, I will try to work on some PoC next week

@jayu jayu self-assigned this May 11, 2020
@jayu
Copy link
Contributor

jayu commented May 12, 2020

After trying to implement composition I understood that it is not possible in a way I thought it would be.
My approach was to grab composed class styles and just inline them inside another class. But if we consider a situation that shared styles are imported from other files, and what's worse, it imports another file, we end up with a need to resolve all styles during preevaluation, which will kill the performance at all and would be very hard to implement.

So the approach we can take is similar to what CSS-Modules do with composes: property. We can compose css classes that have some limitations.
It could work like described here https://webpack.js.org/loaders/css-loader/#composing

Since we have merge class names, rules from composed class names will override corresponding rules in base class regardless of the ordering, so

const shared = css`
   color: red;
`
const base = css`
   ${shared}
   color: blue;
`

will work the same as

const shared = css`
   color: red;
`
const base = css`
   color: blue;
   ${shared}
`

Which will result in blue color due to overridden value in CSS
I need to check how exactly composes works in CSS modules, but In each example that I found it is at the beginning of the declaration, so I guess we have to do the same.
Eventually, we can distinguish between interpolation of shared at the begging and at the end of css definition, but it brings additional complexity .

I guess this not match the behavior from emotion since the code above would act differently in Linaria
Overriding the same prop is an egde case, but may break a lot of styling, unfortunately. How it will work with you use case @jonatansberg?

It doesn't matter wheter we use composes or extends keyword or just plain interpolation, so we can make syntax like in emotion or like in css-modules or even both. But having composes keyword explicitly makes it easier to reason about in terms of how it works.

Update : We have to have composes: keyword explicitly because, without it, we cannot distinguish between object/string being interpolated vs class name during the pre-evaluation.

Nested interpolation
would no be allowed so it will not work

const base = css`
   color: blue;
   .nested {
       ${shared}
   }
`

@jonatansberg
Copy link

@Jayphen: Does this help in any way?

@Jayphen
Copy link
Contributor

Jayphen commented May 12, 2020

The behaviour of overriding properties regardless of the interpolation order is fine — although it's not exactly equivalent to the spec CSS behaviour, it's the intended outcome when "mixing in" styles this way (at least in the examples I've seen in our projects).

In our case, the interpolation isn't necessarily always at the beginning of the declaration, so if that is a limitation then the codemod would need to 'hoist' the interpolation(s?).

I believe we also have cases where interpolations are nested, and there's no way to codemod that, which will still be an issue.

edit: I see you updated your post @jayu — what would the syntax look like with the composes keyword?

@jayu
Copy link
Contributor

jayu commented May 12, 2020

@Jayphen So with my current approach it could look like this :

const shared = css` 
  color: peachpuff;
`;

const otherStyles = css`
  composes: ${shared}
  background: crimson;
`

const InlineComponent = styled.span`
  text-transform: uppercase;
`

const Component = styled.div`
  padding: 1rem;
  composes: ${otherStyles}

  ${InlineComponent} {
    font-weight: bold;
  }
`

And effectively it would act like composes is hoisted to the top of declaration, so styles from particular tag will override the composed styles because of the class names composition.
So you can put composes: anywhere in the tag, but styles from the tag will have priority

@jayu
Copy link
Contributor

jayu commented May 13, 2020

I think I found a way to go with my initial approach to spread the styles... I will test the performance of both solutions.

@jayu
Copy link
Contributor

jayu commented May 19, 2020

I opened a draft PR with PoC implementation, the allowed syntax is as below, please find the description in the PR #615

const fragment = css`
  color: red;
  font-size: 40px;
  padding-top: ${(props) => props.height || '200px'}; 
`

const anotherFragment = css`
  color: yellow;
  ${fragment};
  border: 10px solid black;
`

const component styled.h1`
color: black;
${anotherFragment}
.sub-class {
    ${fragment}
}
`

@kondei
Copy link

kondei commented Apr 19, 2021

Are there any updates?
I want a composition function.
I'm reluctant to use styled-components because linaria doesn't have it.
Without CSS-in-JS side composition for deterministic behavior, We have to fight against non-deterministic behavior of CSS classes that depend on the uncontrollable order in which the bundler outputs them.

@jayu
Copy link
Contributor

jayu commented May 11, 2021

I wish I could have more time to tackle that, but unfortunately, I don't.

Version with overloaded css to be used to generate class name or fragment was rejected and It was a good decision at the end.

The main issue in using any other tag for fragment is syntax highlighting inside the template string, which I believe is not a big blocker for implementing that feature. I did not investigate, but the available tools should support syntax highlighting for any language syntax in that or another way, so we check that first and then agree on the tag name.

Besides the main issue, @kondei if you have a problem with the non-deterministic order of CSS classes in your bundle, you should try using some lint rule to sort your imports, which would guarantee that imports are sorted based on deterministic rules.
That might be painful at the beginning, as many of the styles can break, but there is no other way to tell the bundler in which order it should output the classes into the CSS bundle. For mini-css-extract-plugin there is a famous issue of conflicting-order
webpack-contrib/mini-css-extract-plugin#250. Many people suggest to just ignore the warnings, but in fact, this problem is quite significant for bigger projects with a lot of code, since with non-deterministic order of imports, you can for example swap imports in one component, which would swap the order of CSS output somewhere higher in the dependency tree, in a place in the app, where you would never expect that to happen. It's really hard to track and predict those bugs. Sorting imports is the only way to solve that. Even with fragment tag, you will encounter that problem sooner or later

@brvnonascimento
Copy link

Couldn't we have a suffix on the variable name that would prevent its transformation into a className? For example:

const heading1__fragment = css`
  background: red;
`

@dev-m1-macbook
Copy link

dev-m1-macbook commented Jan 3, 2022

@brvnonascimento
or maybe we can have another tag for that:

const highlightedBackground = cssFragment`
  background: yellow;
`

though I'm wondering whether webpack will tree-shake this without extra work from linaria / etc

@IgnusG
Copy link

IgnusG commented Jul 22, 2022

@brvnonascimento or maybe we can have another tag for that:

const highlightedBackground = cssFragment`
  background: yellow;
`

Looks like we almost went full circle to the original issue's suggestion 😅

@glomotion
Copy link

glomotion commented Oct 24, 2022

So I have just landed here, after trying to evaluate Linaria as a potential style tooling base for a new DS i'm working on. My team and I had dearly hoped to engineer a props-based style approach much like https://styled-system.com/api. Sadly though, we'd need interpolation of the css fragment within a style block (so that we can axs component props).

Its now become clear, that such an aspiration is not currently possible within a Linaria based project (or am I wrong there?)

If its not possible currently, are there any plans in the roadmap to address this gap between styled-component's API and Linaria's?

@shiro
Copy link

shiro commented May 17, 2023

@glomotion I don't know whether or not the core library will add it in the (near) future, but if you just want to use it today, the following code will just work:

export const style = (s: TemplateStringsArray, ...expr: Array<string | number | CSSProperties>): string => {
    let res = "";
    for (let i = 0; i < Math.max(s.length, expr.length); ++i) {
        res += s[i] ?? "";
        res += expr[i] ?? "";
    }

    return res;
};

// usage
const shared = style`
  border: 1px solid blue;
`;
const someClass = css`
  background: red;
  ${shared}
`

It just turns the contents into a string and gives you IDE completion (at least in IntelliJ due to the word style being handled by the styled component plugin).
Is it a hack? yes...
I've been using it on a pretty huge codebase for 2 years now without issues though.

@mohammedhammoud
Copy link

Any plans do support this?

@Firsh
Copy link

Firsh commented Aug 20, 2024

Since I don't actually use css from linaria in its current form, but the syntax highligter extension in VSCode relies on that syntax I use this for static stuff:

export const css = (strings: TemplateStringsArray): string => strings.join("");

For anything dynamic, I use CSS vars or don't put them inside css`` at all. This is like a mixin to avoid repeating myself when simple inheritance is not enough and want to compose something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core team 🧬 Issues reserved for the core team feature: proposal 💬 New feature proposal that needs to be discussed
Projects
None yet
Development

No branches or pull requests