Remove attrs/attrs-arg-access #19660

nlfurniss · 2021-07-19T03:10:42Z

Part of #19617

Deprecation Guide

packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js

packages/@ember/-internals/glimmer/tests/integration/helpers/closure-action-test.js

mixonic · 2021-08-04T01:10:52Z

packages/ember-template-compiler/lib/plugins/transform-attrs-into-args.ts

-          );
+          if (node.this === false) {
+            assert(
+              `Using {{attrs}} to reference named arguments has been deprecated. {{attrs.${


If we added an assertion:

Suggested change

`Using {{attrs}} to reference named arguments has been deprecated. {{attrs.${

`Using {{attrs}} to reference named arguments is unsupported. {{attrs.${

And this test should be renamed to assert-against-attrs in the style of https://github.com/emberjs/ember.js/pull/19691/files, it is no longer a transform. The transforming behaviors here should be dropped.

However this removal does not need to add an assert against {{attrs}}. We can just drop the transform.

The suggestion in my last review was that a template lint could be added to ember-template-lint against {{this.attrs}}. Once we remove the deprecation in this PR, {{this.attrs}} will Just Work in Ember components like any other property access. We should lint against it, but we cannot break it. That would make it special, and our goal is to have fewer special things.

It doesn't seem (based on a comment) that the transform is not needed. Also, if I comment out the transform, tests that use this.attrs.foo fail with

Expected: "In layout - someProp: something here" Result: "In layout - someProp: [object Object]"

packages/@ember/-internals/glimmer/tests/integration/application/engine-test.js

…losure-action-test.js Co-authored-by: Matthew Beale <matt.beale@madhatted.com>

mixonic · 2021-08-17T02:53:13Z

Thanks for all the work on this @nlfurniss

@foo

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)

@foo

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)

@foo

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)

@foo

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) (cherry picked from commit dba5b9a)

nlfurniss mentioned this pull request Jul 19, 2021

Remove EMBER_EXTEND_PROTOTYPES #19661

Merged

mixonic mentioned this pull request Jul 19, 2021

Deprecation Cleanup for 4.0 #19617

Closed

58 tasks

mixonic requested changes Jul 20, 2021

View reviewed changes

packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js Show resolved Hide resolved

rwjblue requested changes Jul 21, 2021

View reviewed changes

packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js Show resolved Hide resolved

rwjblue added the Cleanup label Jul 21, 2021

nlfurniss requested review from mixonic and rwjblue August 2, 2021 00:26

mixonic added the Ember 4.0 label Aug 4, 2021

mixonic requested changes Aug 4, 2021

View reviewed changes

nlfurniss and others added 5 commits August 16, 2021 22:41

Remove attrs/attrs-arg-access

baca206

Update packages/@ember/-internals/glimmer/tests/integration/helpers/c…

4a93f80

…losure-action-test.js Co-authored-by: Matthew Beale <matt.beale@madhatted.com>

Update packages/@ember/-internals/glimmer/tests/integration/helpers/c…

e4f73af

…losure-action-test.js Co-authored-by: Matthew Beale <matt.beale@madhatted.com>

Tweak test names

572ae80

Move transform to assertion

7b9e496

mixonic merged commit b818f54 into emberjs:master Aug 17, 2021

nlfurniss deleted the remove-attrs branch August 17, 2021 02:53

chancancode mentioned this pull request Mar 28, 2024

[BUGFIX lts] Restore {{this.attrs.foo}} deprecation #20672

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove attrs/attrs-arg-access #19660

Remove attrs/attrs-arg-access #19660

nlfurniss commented Jul 19, 2021

mixonic Aug 4, 2021

nlfurniss Aug 4, 2021

mixonic commented Aug 17, 2021

	`Using {{attrs}} to reference named arguments has been deprecated. {{attrs.${
	`Using {{attrs}} to reference named arguments is unsupported. {{attrs.${

Remove attrs/attrs-arg-access #19660

Remove attrs/attrs-arg-access #19660

Conversation

nlfurniss commented Jul 19, 2021

mixonic Aug 4, 2021

Choose a reason for hiding this comment

nlfurniss Aug 4, 2021

Choose a reason for hiding this comment

mixonic commented Aug 17, 2021