Remove attrs because they were deprecated for removal in 4.0

[NullVoxPopuli]

Unblocks the GlimmerVM upgrade: #20658

Merged RFC: emberjs/rfcs#690
Deprecation guide: https://deprecations.emberjs.com/v3.x#toc_attrs-arg-access

[kategengler]

Did we actually deprecate this.attrs or just attrs? Asking cause I'm looking at an app on 5.x that has this.attrs in some places and seeing no deprecations.

[NullVoxPopuli]

I think the deprecation of {{attrs}} pre-dates usage of this (or is very close to being before this was standard way to access things) -- but the deprecation guide mentions moving away from {{this.attrs}}. Since we only throw the deprecation glimmer AST plugin, I think it was an oversight during implementation, or the timing was just bad.

[kategengler]

I think the problem here is that there is this.attrs in apps in the wild (and maybe attrs) and it is just working and hasn't even been displaying a deprecation.

[NullVoxPopuli]

I'm going to propose we remove immediately via RFC

[NullVoxPopuli]

we have a build time error without the this:

 Assertion Failed: Using {{attrs}} to reference named arguments is not supported. {{attrs.foo}} should be updated to {{@foo}}.

I think this is a bug in the transform:

so... like.. folks should have stopped using attrs anyway

[NullVoxPopuli]

it's possible this was due to a lack of coverage and was just missed as the VM's been upgraded

[NullVoxPopuli]

https://github.com/emberjs/rfcs/pull/1016/files

[kategengler]

I just found this.attrs.foo in a project on 5.5.0 and it was working fine with no deprecations issued 😬

[NullVoxPopuli]

oh no 🙈

[chancancode]

There is some confusion/things being conflated here.

this.attrs is a real property on classic component classes that, afaik, isn’t deprecated. You can access those from the JS side just fine and do whatever you want with them, with the caveat that the content of this.attrs.* contains the wrapper mut cell objects which may or may not be what you want/would prefer to work with.

But when you reference them in the template you probably didn’t meant to render [object Object] and at one point the decision was made to handle this automatically so that {{attrs.foo}} and {{this.attrs.foo}} are basically internally aliases as {{@foo}}. Yes at the time explicit this was not the norm but you could always have added that explicitly.

When we deprecated implicit this we also have to deal with this as it is a special case on top of normal property access in the templates and we just told people to use {{@foo}} instead.

In the post-deprecation-removal world, imo {{this.attrs.foo}} should give you the same mut cell object that you would get if you were to do the same in the JS side. Whether that would need a different deprecation is a matter of practicality.

But we aren’t talking about deprecating classic components having an attrs object in the first place, nor are we talking about deprecating accessing it from JS (not even deprecating accessing it from the template necessarily, just that if you think it unwraps for you now that will stop being true).

But I actually don’t think we intentionally did anything in the VM that should/would break this test. There is a good chance that one of the changes I did (probably to the PathExpression node) is accidentally buggy, perhaps node.this is not having the right value etc. So I would dig deeper into why it stopped working regardless of whether we deprecate this

[NullVoxPopuli]

no worries, I made a deprecation RFC: emberjs/rfcs#1016

ontains the wrapper mut cell objects which may or may not be what you want/would prefer to work with

ah, so the glimmer-vm upgrade PR needs another build-time plugin that converts this.attrs.x to this.attrs.x.value (or maybe it already exists and I missed it, and it needs updating)

[chancancode]

ah, so the glimmer-vm upgrade PR needs another build-time plugin that converts this.attrs.x to this.attrs.x.value (or maybe it already exists and I missed it, and it needs updating)

Given the existing test, that must already be the case somewhere and I think something unintentionally broke it, which should be investigated regardless of whether we intend to keep it around, because something else unrelated would probably be affected by the same underlying bug as well

[chancancode]

Ok I see what’s happening now. It’s all in the screenshot you pasted above.

I can’t tell you if this was intended or a bug when converting the AST plugin from transforming the access to an assertion, but notice how the isAttrs (which sounds like it is just checking stuff) actually mutates the node.

If I am reading it right it, intentionally or not, end up changing this.attrs.foo to this.foo, which is why the tests was kept working before.

It used to be that the node.this flag is completely independent of node.original. In an attempt to keep them in-sync, I made more things into getter/setter so that when you assign node.original it detects/infers and sets node.this for you.

Perhaps that turned out to be a little wrong. Based on what this existing plugin is doing, it may be the case that the this. is never part of node.original, hence the change.

The plugin here is problematic for sure, if only just because the mutation is hidden away and makes it hard to understand, but I think there is a potential ast breakage to correct

[NullVoxPopuli]

yeah, I'm gonna fix it after I finish the action deprecation PR (it's big)

[chancancode]

Superseded by #20672