[3.17.0 beta regression] Positional array arguments in helpers have changed in some cases

[patocallaghan]

I've found an issue which was introduced with the Glimmer VM upgrade in v3.17.0-beta.1 . The only changelog item in that release is:

#18688 / #18621 Updates Glimmer-VM to v0.46

I found this issue through our use of ember-simple-set-helper. To reproduce we have the following code where we pass the set action an object property to a child component and then set the property in the child:

{{!-- app/templates/components/parent.hbs --}}
<Child @onChange={{set @user.name}} />

{{!-- app/templates/components/child.hbs --}}
<button type="button" {{on "click" (fn @onChange "Bob")}}>Click to become Bob!</button>

where @user is a simple EmberObject (in my app it's an Ember Data model).

import EmberObject from '@ember/object';

export default EmberObject.extend({
  name: ''
})

In ember-source v3.16.3 this works as expected and user is updated correctly. But from ember-source v3.17.0-beta.1 (and up to beta.6) this throws the following assertion from ember-simple-set-helper:

you must pass a path to {{set}}

Looking at how the {{set}} works it appears that how the positional params are passed into the helper has changed, which I imagine is because of the glimmer-vm upgrade.

v3.16.3

Here we see the first param in the positional array has extracted out the @user target correctly.

v3.17.0-beta.1

But here we see that the @user target is not extracted and is undefined. This causes the helper to throw an assertion on the next line.

I have a reproduction test in pzuraq/ember-simple-set-helper#5

/cc @pzuraq

[pzuraq]

This is most likely a bug in ember-simple-set-helper since it uses a custom AST transform, which is likely now broken. The AST has changed a decent amount in this VM upgrade.

Edit: For context, the AST is not considered public API and changes like this are considered to be within semver.

[patocallaghan]

@pzuraq ah cool, thanks for the context. I'll close this issue then and open one over there. I'll see if I can put together a fix too.

[patocallaghan]

@pzuraq So I've dug into this and I'm reopening the issue because I'm actually not so sure this is a bug specific to ember-simple-set-helper. You may have more context on the how the transformation pipeline works so please close if you think this is definitely not a glimmer-vm bug. At the very least I'll dump here what I've discovered.

So overall it seems like the AST is operating as expected. The AST is being converted to the following in ember 3.17.0-beta.6 if I put a breakpoint into the transform file.

Note that {{set @user.name}} is converted to {{set @user "name"}}. But once the set helper is called @user is undefined in the positional params.

Some other things I tried to try and figure out what is going on include:

1. Trying {{set this.localUser.name}} instead of an argument where this.localUser is a property on the component. This work as expected and the first positional is correctly this.localUser. So it seems to be happening specific to arguments.
2. I also created my own helper which implemented exactly what the final transformed output state of ember-simple-set-helper is, i.e. {{my-set @user "name"}}. This also correctly passed the positional params to the helper with @user being the passed in User object.

So to put it mildly I'm a bit confused as to what's happening here 😅 Is there an order thing happening here where the {{set}} transform happens too late that it doesn't have access to the @user context anymore?

[pzuraq]

There was a tricky issue with the AST transform. It appeared to be working, but it wasn't semantically the same output. Glimmer know cares about whether or not a value is a data property (an argument) or a path, and so the AST transform needs to take that into account.

I updated your PR on ember-simple-set-helper to fix the issue, should be able to merge and release soon. Going to close this, thank you for reporting and digging in!

[patocallaghan]

Amazing, thank you 🙇