Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 29, 2024

Conversation

chancancode
Copy link
Member

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, 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:

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.

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)
// in the template since it is a real property on the backing class – it will give you
// a `MutableCell` wrapper object, but maybe that's what you want. And in any case,
// there is no compelling to special case that property access.
deprecate(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a new thing somewhat recently -- you should use deprecateUntil from @ember/-internals/deprecations and also define the deprecation in there (there are docs in there).

Copy link
Member Author

@chancancode chancancode Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, it will make it slightly more work to back port, but it shouldn't be too bad.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to revert this – the build wasn't properly set up for ember-template-compiler to be able to import from @ember/-internals. I think it can probably be made to work, but it would require some refactoring and ensure that @ember/-internals doesn't accidentally depend on browser features since it'll also have to run in node.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given my other question #20669, I do wonder if we will be better off enforcing that at build time in wherever-that-babel-code-lives.

In any case, I think we can land this and come back to fix it later when/if that other problem gets resolved. It should also make it easier to back port in the meantime. I can add a manual assertion here for the ember-source version if you would like.

@chancancode chancancode force-pushed the deprecate-this-dot-attrs branch 3 times, most recently from e724cac to dba5b9a Compare March 28, 2024 23:40
@kategengler
Copy link
Member

I think this is fine, especially if we are backporting. We should definitely figure out using deprecateUntil in ember-template-compiler though because we want to avoid having a bunch of work at the major version boundary (and want to test with and without deprecations "removed").

@chancancode chancancode merged commit 1a4dd6a into main Mar 29, 2024
44 checks passed
@chancancode chancancode deleted the deprecate-this-dot-attrs branch March 29, 2024 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants