wix / stylable

Stylable - CSS for components
https://stylable.io
MIT License
1.27k stars 62 forks source link

@stylable/core v2.0.0 #486

Closed tomrav closed 5 years ago

tomrav commented 5 years ago

Upcoming major version for Stylable, including breaking changes.

Core syntax

CSS Custom Properties (vars) - #247

Runtime API

)}

```html
<!-- result -->
<div className="comp__root app__comp">
    <span className="comp__inner comp--active unscoped">
        hello world
    </span>
    <span className="comp__other">
        goodbye world
    </span>
</div>

Webpack-plugin

TypeScript Stylesheet Module Type

CLI

Node integration

New Packages

Project infrastructure

Note: We will be creating a migration guide soon, but until then I have created a PR in our potato-bruce sample repo that can be used as a reference for changes needed.

Scott-MacD commented 5 years ago

Just curious about the change to use classes instead of data-* attributes you guys are making and how that api might work. Does that mean that the stylesheet returned in the runtime will automatically have these state classes applied when you call style.myComponent for example? If so, what are the thoughts on what the JS API is going to look like to control what the state is?

This change sounds like it might simplify integration with hyperHTML if we're only updating the class attribute and not dynamically named data attributes.

idoros commented 5 years ago

Hey @Scott-MacD, I'm not sure what you mean by:

runtime will automatically have these state classes applied when you call style.myComponent

States are never "on" by default, so style.myComponent will still return the normal namespaced class name.

@tomrav - correct me if I'm wrong, The only change should be that calling style('myComponent', {stateA: true}) returns {className:'NS_myComponent NS-stateA'}. The className attribute is for backwards compatibility here, but a new named method should be exported: $part('myComponent', {stateA: true}) that returns just the class attribute value: NS_myComponent NS-stateA. My bad, this is a major version because of this. Calling style('myComponent', {stateA: true}) should return only the result class attribute value: NS_myComponent NS-stateA.

This change sounds like it might simplify integration with hyperHTML if we're only updating the class attribute and not dynamically named data attributes.

That's right! The reason for setting the states as class names instead of data attributes is to simplify the way Stylable is applied to the DOM, making it simpler to integrate into components.

Together with CSS vars support Stylable will work with the class attribute and optionally the style attribute. So instead of spreading unknown attributes, Stylable will only require the class name. The change should keep the same specificity since class names and attribute selectors are equal.

FYI, The reason we choose to represent states as data-* attributes, to begin with, was for support of more custom states that we never actually implemented, and just don't seems to be worth it for now. You can see some of the issues describing such states (can be deleted/archived now):

Scott-MacD commented 5 years ago

Thanks for the explanation @idoros! Between that change and the better support for custom properties I'm very much looking forward to this release.

Thanks for the great work as always Wix Engineering team!

tomrav commented 5 years ago

@idoros was pretty much spot on in his explanation of our intent. Basically, simplify the API and try not to touch things that don't belong to us or we don't fully control, limiting our interaction to two attributes - class (className) and style.

Here are some examples of the new CSS output

/* entry.st.css */
.root {
    -st-states: booleanState,
                stringState( string ),
                numberState( number ),
                enumState( opt1, opt2, opt3 ),
                tagState( tag );
}

/* Boolean */
.root:booleanState {}      /* transpiles to */
.entry--root.entry--booleanState {}

/* String */
.root:stringState(val) {}  /* transpiles to */
.entry--root.entry--stringState-val {}

/* Number */
.root:numberState(42) {}   /* transpiles to */
.entry--root.entry--numberState-42 {}

/* Enum */
.root:enumState(opt1) {}   /* transpiles to */
.entry--root.entry--enumState-opt1 {}

/* Tag */
.root:tagState(someTag) {} /* transpiles to */
.entry--root[class~="entry--tagState-someTag"] {}

If any of the states above received a parameter value that would contain illegal classname syntax it would instead get transpiled to a more lenient selector.

.root:stringState(bro.ken) {} /* transpiles to */
.entry--root[class~="entry--stringState-bro.ken"] {}

Manual state mapping - keeps working as before.

.root {
    -st-states: mappedState1(".on"),
                mappedState2("[data-state]")
}

.root:mappedState1 {}       /* transpiles to */
.entry--root.on {}

.root:mappedState2 {}       /* transpiles to */
.entry--root[data-state] {}

And examples of applying state in your component:

import style from "./entry.st.css";

const Comp1 = (props) => <div className={ style('root') } />; // returns 
<div className="entry--root" />

const CompWithBoolean = (props) => {
    return <div className={ style('root', { booleanState: true }) } />
}; // returns 
<div className="entry--root entry--booleanState" />

const CompWithString = (props) => {
    return <div className={ style('root', { stringState: 'val' }) } />
}; // returns 
<div className="entry--root entry--stringState-val" />

// ... and so on
tomrav commented 5 years ago

...and while we're at it, we can also take advantage of this breaking change to further change the API for classes. Because we are limiting our interaction to the className attribute, we can reduce the number of arguments we receive to two:

  1. class - string or array of strings already namespaced
    • the style function will not perform namespacing, but would rather depend on the string being imported in its namespaced form
  2. state - object defining state on the current node
import style, {root, classes, vars} from "./entry.st.css";

const CompWithString = (props) => { 
    return (
        <div className={ style([root, props.className], { stringState: 'val' }) } >
            <span className={ style[classes.part, 'manual-addition') } />
        </div>
    );
}; 

// returns 
<div className="entry--root entry--stringState-val" >
    <span className="entry--part manual-addition" />
</div>

Another option we can consider is supporting the classNames syntax, to make conditionally joining classes easier.

import style, {root, classes, vars} from "./entry.st.css";

const x = 42;
const CompWithString = (props) => 
    <div className={ style([
            root,
            { foo: true, [classes.part]: x > 0 },
            { nope: false }
        ], 
            { stringState: 'val' }
    )} />; 

// returns 
<div className="entry--root foo entry--part entry--stringState-val" />

Personal note: I don't think we should add the classNames behavior as described above. I suspect it might cause users of Stylable to adopt bad practices instead of using functionality native to the library. I feel if someone were to want that functionality bad enough for their project it would be easy enough to consume as a utility.

Scott-MacD commented 5 years ago

So from my understanding, with these API changes the following should be possible?

Stylesheet:

/* entry.st.css */
.root {
    -st-states: enabled;

    --color: #0F0;
}

.root:not(:enabled) {
    cursor: not-allowed;
    opacity: 0.5;
}

.part {
    color: var(--color);
}

Component JS (example with template-literal instead of JSX):

import style, {root, classes, vars} from "./entry.st.css";

const render = ({enabled, color, text}) => html`
    <div class="${style(root, {enabled})}" style="${vars({'--color': color})}">
        <span class="${classes.part}">${text}</span>
    </div>
`;

render({enabled = true, color: "#F00", text: "Hello world!"});

Should result in the following HTML?

<div class="entry--root entry--enabled" style="--entry-color: #f00">
    <span class="entry--part">Hello world!</span>
</div>

Are state classes still intended to be namespaced per component/stylesheet and not per classname? If so, and the above is true, would it be possible to get state as a standalone export from the stylesheet so that something similar to the following js is possible? (should still use the same .st.css and generate the same result):

import {classes as style, vars, state} from "./entry.st.css";

const render = ({enabled, color, text}) => html`
    <div class="${style.root + state({enabled})}" style="${vars({'--color': color})}">
        <span class="${style.part}">${text}</span>
    </div>
`;

render({enabled = true, color: "#F00", text: "Hello world!"});

Having an object with the child classes as properties is nicer to work with than strings, as it automatically gives a compile time error in all the major existing build tools if you try to access a class that doesn't exist. Having the option to do classes.someClassName and classes.someClassName + state({booleanProp: true}) just feels a lot easier to read the template markup, and provides a bit more consistency to those working on a codebase who might not be as familiar with Stylable.

Scott-MacD commented 5 years ago

If the vars export is specifically being used for css variables it could also be cleaned up further by allowing the omission of the -- prefix to properties, and automatically add them to the generated CSS. This would allow writing:

vars({font: font, color: color, size, size})

// or taking advantage of modern object property shorthand: 
vars({font, color, size})

// resulting string:
"--entry-font: Helvetica; --entry-color: golden; --entry-size: 20rem;"

Which means the example above could become even cleaner as:

import {classes as style, vars, state} from "./entry.st.css";

const render = ({enabled, color, text}) => html`
    <div class="${style.root + state({enabled})}" style="${vars({color})}">
        <span class="${style.part}">${text}</span>
    </div>
`;

render({enabled = true, color: "#F00", text: "Hello world!"});

(See edit for diff of initial example to current example)

tomrav commented 5 years ago

Should result in the following HTML?

<div class="entry--root entry--enabled" style="--entry-color: #f00">
    <span class="entry--part">Hello world!</span>
</div>

That looks right to me, except for one small issue that I too missed. 👍 We'll need some way of separating classes and states one from the other, otherwise here class="entry--root entry--enabled", you'll have a conflict if the stylesheet has a class named enabled. Perhaps prefexing the state name with a $, e.g. class="entry--root entry--$enabled"


Are state classes still intended to be namespaced per component/stylesheet and not per classname? If so, and the above is true, would it be possible to get state as a standalone export from the stylesheet so that something similar to the following js is possible?

For the time being states will remain namespaced per stylesheet, but that might be something we'll re-explore in a later version. However, exposing a way to more easily generate strings symbolizing classes/states definitely seems in line with what we're discussing.


If the vars export is specifically being used for css variables it could also be cleaned up further by allowing the omission of the -- prefix to properties, and automatically add them to the generated CSS. This would allow writing:

The issue with giving up the prefix is that the utility function in question would also like to pass through inline styles passed to it. So if you were to pass color: 'red' it won't be able to tell if that should be a var or an untouched property.


Considering the fact that the final output of our API is now a single string (to fit in a className attribute), a new simpler API is possible:

import style, { states } from 'entry.st.css'

// for the sake of the example:
// props.className = 'app--part';

style(
  style.root,
  props.className,
  states({
    enable1: true,
    enable2: 'val',
    enable3: false
  }),
  'external'
); // outputs:

"entry--root app--root entry--$enable1 entry--$enable2-val external"

In this API example, the style function can receive any number of arguments, where each one must be a string. strings passed will be concatenated and applied as is. The stylesheet will then expose states as a utility function that accepts a list of states and returns a concatenated string of their namespaced values.

idoros commented 5 years ago

We'll need some way of separating classes and states one from the other, otherwise here class="entry--root entry--enabled", you'll have a conflict if the stylesheet has a class named enabled. Perhaps prefexing the state name with a $, e.g. class="entry--root entry--$enabled"

How about just going with BEM:

.entry__part {}
.entry--state {}

/* and maybe in the future: */

.entry__part--state {}
Scott-MacD commented 5 years ago

I personally like the way that API sounds. It would still allow a bit more backwards compatibility with the current version by allowing simpler style.part use when you don't need anything additional on the element, but give the flexibility of extending that functionality in a way that feels iterative and clear with style(style.part, states({enabled: true})) when you need it.

Using BEM as the generated naming convention would also perhaps make it easier to understand for those who might already be familiar with BEM, but looking for a more powerful tool due to enforcement issues, confusion on when to manually make a child component, etc. There's probably a way to highlight this as one of Stylable's strengths in the docs.

The issue with giving up the prefix is that the utility function in question would also like to pass through inline styles passed to it. So if you were to pass color: 'red' it won't be able to tell if that should be a var or an untouched property.

Makes sense. I think leaving the prefix as mandatory makes the most sense here, and the benefit of having it as an exported function means that if somebody has no need to use inline styles and wants to make a wrapper around it to handle that for them, they can. vars is perhaps a misleading name if it's intended to be used for any inline styles - however, I don't have any better ideas off the top of my head.

tomrav commented 5 years ago

vars is perhaps a misleading name if it's intended to be used for any inline styles - however, I don't have any better ideas off the top of my head.

I think that's a good point and feel like we haven't quite nailed the naming of named exports just yet.

The additional named exports might actually only get added in a later minor version, if our chosen API ends up not necessarily needing it. This still requires more work.

tomrav commented 5 years ago

Following another round of discussions on the API, we seem to have settled on a solution that bares some similarity to the existing API, but with a few changes.

We definitely aim to support states per class (probably in addition to stylesheet states) and want our new API to reflect it even now.

A stylesheet still exposes a default export, with the classes and vars mapped on it, as well as the runtime function.

import style from 'entry.st.css';

The common use case of only applying a class to an element will remain the same.

<span className={style.label} />

The runtime function will change to one of the following: Option 1

function style(
  contextClassName: string, statesOrClassName: StateMap | string, ...classNames: string[]
): string

Usage examples:

<div className={style(style.root, { loaded: true }, props.className, 'other-global')} />

<div className={style(style.label, style.highlight)} />

The first argument is a className to be applied to the node, and the context which states will be scoped with in the future. The string passed here is imported from the stylesheet and already scoped. The existing syntax of manually entering un-scoped class names will no longer be supported.

The second argument can be one of two options, either an object representing states with their local un-scoped names, or another className to apply.

Edit: After verifying that this option would not incur de-optimization costs, we're going with option 1.

Option 2

function style(
  contextClassName: string, stateMap: StateMap, classNames: string[]
): string

Usage examples:

<div className={style(style.root, { loaded: true }, props.className, 'other-global')} />

<div className={style(style.label, {}, style.highlight)} />

The first argument behaves the same as it does in Option 1.

The second argument will only represent state, and if you need no states but still require an additional class you'll need to pass it an empty object.

The rest of the arguments behave the same as they do in Option 1.


...and while we do not recommend this approach, both options will support compositions of complex styles. For example:

<div className={style(style.root, { loaded: true }, style(style.dark, { dark: true }))} />
Scott-MacD commented 5 years ago

I believe either of these options covers the use cases I've personally encountered so far in an easy to understand manner.

Just for the sake of completeness, here's what the same example I was posting above would look like with either of these APIs (which arguably is even easier to read):

import style, {vars} from "./entry.st.css";

const render = ({enabled, color, text}) => html`
    <div class="${style(style.root, {enabled})}" style="${vars({'--color': color})}">
        <span class="${style.part}">${text}</span>
    </div>
`;

render({enabled = true, color: "#F00", text: "Hello world!"});
tomrav commented 5 years ago

Update!

I have released a next tagged version of Stylable with all of the changes described above (updated top post, too). I think it is stable enough for user testing to begin.

I will be creating a migration guide in the near future, but until then I have created a PR in our potato-bruce sample repo that can be used as a reference for changes needed.

Scott-MacD commented 5 years ago

I started playing around with the next branch and am happy with the API changes so far! The change from states using data attributes to class names already is making things more pleasant to work with.

I'm actually just starting development on a new internal tool and have switched to using the next branch for the UI for this project. I'll let you know if we encounter any issues along the way, but I'm liking how everything is working together so far!

tomrav commented 5 years ago

Heads up, I just published another version of next, including a new breaking change.

@stylable/webpack-plugin now exposes the StylableWebpackPlugin as a named export (used to be default export), as shown here:

// ESM
import { StylableWebpackPlugin } from '@stylable/webpack-plugin';

// CommonJS
const { StylableWebpackPlugin } = require('@stylable/webpack-plugin');
tomrav commented 5 years ago

All relevant features have been implemented, merged to master, and published to npm. See top comment for all changes included in the version.