[Glimmer 2] Differentiate between bound class and classNames#13372
[Glimmer 2] Differentiate between bound class and classNames#13372chancancode merged 1 commit intoemberjs:masterfrom
class and classNames#13372Conversation
chadhietala
force-pushed
the
failing-sexpr-if
branch
from
70002de to
e5e42fd
Compare
chadhietala
changed the title
class and className
chadhietala
changed the title
class and classNameclass and classNamea
chadhietala
changed the title
class and classNameaclass and classNames
There was a problem hiding this comment.
I think this should be an EmptyObject (not a dictionary) since we do not need delete from it.
There was a problem hiding this comment.
See the comments in these two files for a relatively good explanation:
- https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/empty_object.js
- https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/dictionary.js
ember-metal/dictionary (when used with null parent) creates an EmptyObject then adds an item and deletes it to force the runtime into keeping it in dictionary mode. Since we don't need this object to be a dictionary, we can just use new EmptyObject() directly.
There was a problem hiding this comment.
We probably don't need it to be an actual dict since we know all the fields (like meta). We can just make an actual class for it
There was a problem hiding this comment.
The object is not exposed to users either, so it's probably not very important for it to be an empty object either
There was a problem hiding this comment.
We do want the object to have a fixed shape though even when some of those meta info are not present.
chadhietala
force-pushed
the
failing-sexpr-if
branch
from
e5e42fd to
315a71e
Compare
|
This is going to fail until we bump glimmer but figured I would post it. update: Glimmer has been bumped. |
There was a problem hiding this comment.
Hm, the original test explicitly claim to care about ordering – and the new test does not (via the classes helper which sorts them). Like I mentioned above, I don't think we have a reason to care, but if for some reason we have I think the test will still pass (you would use a string instead of classes which does an exact match). @rwjblue can we drop this?
There was a problem hiding this comment.
chadhietala
force-pushed
the
failing-sexpr-if
branch
from
021ece6 to
33a67bb
Compare
chadhietala
force-pushed
the
failing-sexpr-if
branch
2 times, most recently
from
9f52d57 to
1b36258
Compare
|
I would like to 💣 that whole test file as I don't believe the first test is valid any more as it's testing |
chadhietala
force-pushed
the
failing-sexpr-if
branch
2 times, most recently
from
4ff61a9 to
e12cc37
Compare
3 participants
This fixes a behavior where a bound
classcan mutate, where as boundclassNamesare const'd post creation. Also migrating some of the attr_node tests that were testingclassbehavior.