Commits on Mar 28, 2024

1. [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)

   

   chancancode committed Mar 28, 2024
   Configuration menu

   • View commit details
   • Copy full SHA for dba5b9a
   • Browse repository at this point

   Copy the full SHA

   dba5b9a View commit details

   Browse the repository at this point in the history