linkedin / css-blocks

High performance, maintainable stylesheets.
http://css-blocks.com/
BSD 2-Clause "Simplified" License
6.34k stars 152 forks source link

Error with inherited state in htmlbars template #203

Open dacoho opened 5 years ago

dacoho commented 5 years ago

I get the following error with the Ember addon when I try to extend a block with an state:

TemplateError: Can not apply multiple states at the same time from the exclusive state group ":scope[state|error]"

/* block-1.block.css */
:scope[state|error] {
    ...
}

/* block-2.block.css */
@block-reference block1 from "block-1.block.css";
:scope {
    extends: block1
}

/* block-2.hbs */
<div state:error={{error}}></div>

When the state is not inherited, the build is fine:

/* block-1.block.css */
:scope[state|error] {
   ...
}

/* block-1.hbs */
<div state:error={{error}}></div>
amiller-gh commented 5 years ago

Hm. Thats interesting. Will take a look asap, but it may be another week before I can get it on my docket. Think you could pop up a branch with a failing test case?

dacoho commented 5 years ago

Sure! This case fails: https://github.com/dacoho/css-blocks/commit/b88d28caf7b4c5620a427bd8093a9968cd6025d9

amiller-gh commented 5 years ago

Amazing 🎉 Now that I'm back from the Node.js collab summit I'll dig in to this a bit.

dacoho commented 5 years ago

Hi @amiller-gh, I've been reviewing this and have found that in the glimmer analysis, inherited static and dynamic attributes are being added to each element several times - once for each of the block classes.

That happens in this loop: https://github.com/linkedin/css-blocks/blob/7079a5320f2b29f31e246c0c1476de40af35512c/packages/%40css-blocks/glimmer/src/ElementAnalyzer.ts#L218

When attributes added only once the bug disappears. e.g.

let applied = analysis.element.addedStyles.find(style => style.value == state);
if (!applied) {
    analysis.element.addDynamicAttr(container, state, null);
}

Would this be a good solution?

amiller-gh commented 5 years ago

Hey @dacoho, sorry for the slow reply – great sleuthing. That seems like a solid solution, but can we maybe add the existence check to addDynamicAttr so Analyzers don't need to worry about this logic? My hope is for Analysis integration to be as straightforward as possible, if we already made this mistake in one of our own analyzers then it sounds like something that @css-blocks/core can help validate for us!

We can keep a faster model for style presence check on ElementAnalysis instead of iterating through addedStyles each time. addDynamicAttr (as well as all the other style application methods: addDynamicClasses, addStaticClass, addDynamicGroup, addStaticAttr) would use that to check if a Class or Attr has already been added. If yes, no-op, if no, add the Style(s).

GCheung55 commented 4 years ago

I'm seeing an error with states and Glimmer component in an ember-engine that may be related.

Cannot select attributes other than states: :scope[is-on]

Component block CSS

:scope {
    font-weight: bold;
}

:scope[is-on] {
    color: red;
}

Glimmer Component hbs.

<button block:scope block:is-on={{isOn}} {{on "click" this.toggleOn}}>
    {{yield}}
</button>
chriseppstein commented 4 years ago

@GCheung55 You're using the new syntax for 1.0 but the version of css-blocks that you've got installed is 0.24. You can either install @css-blocks/ember-cli@next or use the older syntax in your glimmer templates. I'm working on a new release for @next with a lot of bug fixes right now... it should be out in the next day or two.

GCheung55 commented 4 years ago

@chriseppstein switching to 1.0 worked for me. Thanks!