Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
[BUGFIX lts] Restore
{{this.attrs.foo}} deprecation
Previously (as of 3.28) `{{this.attrs.foo}}` in the template was
deprecated with a suggestion to switch to `{{@foo}}`, since that
is what we internally rewrite that syntax into.
When the deprecation was [removed in 4.0][1], we correctly made the
observation `this.attrs` is a real property on classic component
classes, and accessing it from the template is perfectly legal and
should not trigger any deprecation.
However, what slipped everyone's mind is that `this.attrs.*` gives
you a "Mutable Cell" object. That is not, inheriently problematic.
While it's not particularly useful to render these objects directly
(it stingifies into `[object Object]`), you can render the `.value`
property or perhaps pass them around as values.
What slipped everyone's mind however, is that that's not what the
previously-deprecated `{{this.attrs.foo}}` does or mean – it gets
rewritten into `{{@foo}}`, so in fact, `{{this.attrs.foo}}` is a
magic syntax that automagically unwraps the Mutable Cell for you,
which is indeed unexpected and the whole point of that originally
deprecation.
One reason why this slipped our minds and why it was so hard to
spot was that the rewrite logic is hidden deeply/inconspicuously
as a side-effect inside the `isAttrs()` function.
The original intent (and probably too "clever") of the code was
probably that: at this point, we already know we are going to
throw away and rewrite this node as `{{@foo}}`, we may as well
normalize away any differences between `{{attrs.foo}}` and
`{{this.attrs.foo}}` to make it easier to write the code for
the deprecation message.
Note that even with this generous reading, the original code is
_still_ incorrect. `node.original.slice(5)` is presumably trying
to remove `this.` from the string, and `node.parts.shift()` is
presumably trying to do the same. The latter, however, is wrong
and unnecessary. `node.parts` does not contain the `this` part
to begin with, so this ends up removing `attrs` from the array
instead.
This mistake wasn't consequential in the original deprecated
code, because, as mentioned, the code is going to replace the
`{{this.attrs.foo}}` node with a newly built `{{@foo}}` node
anyway, and the original node is only used for the purpose of
assembling the deprecation message, which doesn't do anything
with `node.parts`.
When the deprecation was removed though, this side-effect ends
up mattering. This modification is an undefined behavior – it
left `node.original` and `node.parts` disagreeing with each
other (ironically, that was because the original author tried
to keep them in-sync, just end up doing it incorrectly).
The latest version of glimmer-vm will ensure these kind of
modifications stay in-sync, but prior to that, it seems like
we end up favoring `node.parts` and so, seemingly without
anyone noticing, the code ends up sneakily transfoming
`{{this.attrs.foo}}` into `{{this.foo}}`, which was never the
intent of any version of this code, but kind of works (for
classic components at least) and went unnoticed so far.
This wasn't caught in the test, because `assertTransformed`
doesn't do what you think. It may sound like that you are
giving it the untransformed code (LHS) and assert that, after
running the AST, it matches exactly the expected result (RHS).
However, that is not at all what it does. The implementation:
```js
this.assert.deepEqual(deloc(ast(before)), deloc(ast(after)));
```
As you can see, it does exactly the same thing with the LHS
and RHS. All it is doing is confirming that either version of
the source code will ultimately normalizes into the same
result after running the AST plugins. Which is to say, all
existing tests that checks `assertTransformed("blah", "blah")`
is testing exactly nothing, because of course compiling the
identical template twice will give you the same result.
Those tests need to be corrected. Or better yet, change the
implementation of `assertTransformed` to do what you would
expect, so that we can _actually_ test no-op transforms. This
commit doesn't address that and that's left for someone else
to pick up.
This commit refactors the code for clearity and reintroduces
the original deprecation with a 6.0 removal. We probably
could have just called it a bugfix, but with the next major
on the horrizon anyway, it doesn't hurt to give this a proper
deprecation cycle in case someone still uses it.
Note that: this has nothing to do with accessing and using
`this.attrs.foo` from the JavaScript class in JS context,
which is not being deprecated here.
[1]: #19660 (comment)- Loading branch information
1 parent
f847f79
commit e724cac