[Glimmer2] Fix class bindings #13337
Conversation
| @@ -1,5 +1,7 @@ | |||
| import { tokenize } from 'simple-html-tokenizer'; | |||
|
|
|||
| let whitespace = new RegExp('( )+', 'g'); | |||
There was a problem hiding this comment.
const WHITESPACE = /\s+/;
The global flag isn't needed for split, neither is the group.
There was a problem hiding this comment.
@krisselden your example includes other things, space\tab\carriage return\new line\vert tab/form feed, which may or may not be expected.
I think the goal is one or more spaces which I think is more accurately expressed as
/ +//[ ]+/or to be clear we want one or more of a space
but ( )+ seems to imply capturing and g is global, both likely not the intent, and /\s+/ includes other chars, which may or not want to be matched.
|
I'm not sure if this the approach we want to take. After talking @wycats it seems like glimmer needs to expose a |
|
☔ The latest upstream changes (presumably #13372) made this pull request unmergeable. Please resolve the merge conflicts. |
asakusuma
force-pushed
the
g2-cb
branch
2 times, most recently
from
ea31281 to
77f9814
Compare
|
Looks like saucelabs firefox is crapping out |
|
Build is passing now |
| let syntax = HelperSyntax.fromSpec(['helper', ['-class'], [truthy], null]); | ||
| args.named.add('class', syntax); | ||
| } else if (falsy) { | ||
| let parts = prop.split('.'); |
There was a problem hiding this comment.
Can't you read the parts of the Ref syntax?
There was a problem hiding this comment.
@chadhietala in this case the object returned from args.named.at('classBinding') only has type and value properties. There's no ref property.
|
@rwjblue I just rebased and now the tests are failing because of a deprecation being thrown as an error. It's unclear to me if this is expected, and if so, does that mean that classBinding should not be implemented for glimmer? Looks like this PR introduced the deprecation: #13424 |
|
@asakusuma It needs to be implemented as |
|
All tests for |
|
@chadhietala I think we were overcomplicating this. Updated PR is now a lot simpler. Let me know if this is not a public API, but seems public to me. |
|
This is creating the binding in the wrong scope. It is supposed to be the template scope not the component. Unfortunately the test does not actually cover bindings, just static classes, but doing a quick search of github, this does seemed to be a used feature though undocumented and not tested. |
| let syntax; | ||
| if (args.named.has('class')) { | ||
| // If class already exists, merge | ||
| let classValue = args.named.at('class').value; |
There was a problem hiding this comment.
This currently won't work with helpers. If we need to support this, we'll need a way to generate a spec from a helper.
|
Thank you @asakusuma! |
Not sure this is correct way to get the class name out of the binding object.