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 Beta] Fix for auto-mut wrapping of closure actions. #13996

Merged
merged 2 commits into from
Sep 29, 2016

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Aug 2, 2016

Given the following:

{{x-foo derp=(action 'herk')}}
{{! app/templates/components/x-foo.hbs }}

{{x-bar derp=derp}}
export default Component.extend({
  didInsertElement() {
    this.get('derp')();
  }
});

An error is triggered with the following output:

TypeError: this.attrs.derp is not a function
    at Class.fireAction (http://localhost:7200/ember-tests.js:21306:46)
    at http://localhost:7200/ember-tests.js:21344:24
    at Object.run (http://localhost:7200/ember.debug.js:294:25)
    at Object.run [as default] (http://localhost:7200/ember.debug.js:19515:27)
    at _class2.runTask (http://localhost:7200/ember-tests.js:32041:34)
    at _class2.testActionClosureDoesNotGetAutoMutWrapped [as @test action closure does not get auto-mut wrapped] (http://localhost:7200/ember-tests.js:21343:12)
    at Object.<anonymous> (http://localhost:7200/ember-tests.js:31691:31)
    at runTest (http://localhost:7200/qunit/qunit.js:843:28)
    at Object.run (http://localhost:7200/qunit/qunit.js:828:4)
    at http://localhost:7200/qunit/qunit.js:970:11

homu added a commit to DockYard/ember-route-action-helper that referenced this pull request Aug 2, 2016
Add ember-alpha to try config.

Tests are passing (with the tweak to avoid using `this.attrs` in the acceptance test).

I have reported the issue that passing an action through a middle layer doesn't properly prevent mut-wrapping in `this.attrs` as was done in HTMLBars with a failing test in emberjs/ember.js#13996.
@chancancode
Copy link
Member

Something seems fishy here, why is any new code accessing this.attrs.* o_0?

@chancancode
Copy link
Member

Ah it's in the test, carry on

@rwjblue rwjblue added this to the Glimmer2-Beta milestone Aug 19, 2016
@rwjblue rwjblue modified the milestones: 2.9.0, Glimmer2-Beta Sep 15, 2016
@chadhietala chadhietala changed the title [GLIMMER] Failing test showing auto-mut wrapping of closure actions. [Bugfix Beta] Failing test showing auto-mut wrapping of closure actions. Sep 29, 2016
@chadhietala chadhietala changed the title [Bugfix Beta] Failing test showing auto-mut wrapping of closure actions. [Bugfix Beta] Fix for auto-mut wrapping of closure actions. Sep 29, 2016
@@ -51,7 +51,9 @@ class SimpleArgs {
let ref = namedArgs.get(name);
let value = attrs[name];

if (ref[UPDATE]) {
if (typeof value === 'function') {
attrs[name] = value;
Copy link
Member Author

Choose a reason for hiding this comment

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

This would prevent passing a class down to a child component. Given the following setup (we should add a test for this):

{{foo-bar klass=ModelClass}}
{{! parent component }}
export default Ember.Component.extend({
  init() {
    this._super();
    this.klass = Ember.Object.extend();
  }
});
// foo-bar component

export default Ember.Component.extend({
  actions: {
    updateKlass(newKlass) {
      this.attrs.klass.update(newKlass);
    }
  }
});

@@ -51,7 +51,9 @@ class SimpleArgs {
let ref = namedArgs.get(name);
let value = attrs[name];

if (ref[UPDATE]) {
if (typeof value === 'function') {
Copy link
Member Author

Choose a reason for hiding this comment

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

In prior versions (1.13 - 2.8) we use a symbol stamped on the generated action function that we used as an identifier that this is an action.

Assuming we do the same here (this would alleviate the concern about intentionally passing down a class / function that is intended to be two way bound), this would look something like:

if (typeof value === 'function' && value[ACTION]) {
  attrs[name] = value;
} else if (ref[UPDATE]) {
  // ...snip...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed.

Given the following:

```hbs
{{x-foo derp=(action 'herk')}}
```

```hbs
{{! app/templates/components/x-foo.hbs }}

{{x-bar derp=derp}}
```

```js
export default Component.extend({
  didInsertElement() {
    this.get('derp')();
m }
});
```

An error is triggered with the following output:

```
TypeError: this.attrs.derp is not a function
    at Class.fireAction (http://localhost:7200/ember-tests.js:21306:46)
    at http://localhost:7200/ember-tests.js:21344:24
    at Object.run (http://localhost:7200/ember.debug.js:294:25)
    at Object.run [as default] (http://localhost:7200/ember.debug.js:19515:27)
    at _class2.runTask (http://localhost:7200/ember-tests.js:32041:34)
    at _class2.testActionClosureDoesNotGetAutoMutWrapped [as @test action closure does not get auto-mut wrapped] (http://localhost:7200/ember-tests.js:21343:12)
    at Object.<anonymous> (http://localhost:7200/ember-tests.js:31691:31)
    at runTest (http://localhost:7200/qunit/qunit.js:843:28)
    at Object.run (http://localhost:7200/qunit/qunit.js:828:4)
    at http://localhost:7200/qunit/qunit.js:970:11
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants