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

cannot alway set a new value to a oneWay property #12953

Closed
ducarroz opened this issue Feb 12, 2016 · 11 comments
Closed

cannot alway set a new value to a oneWay property #12953

ducarroz opened this issue Feb 12, 2016 · 11 comments

Comments

@ducarroz
Copy link

My understanding of a oneWay computed property is that it represents a one way binding from upstream to downstream. And if you set the value of the property, the binding will be torn down and upstream not affected.

Starting from Ember 1.11, some time is not possible to set the value of a oneWay computed binding. Here is an stripped down example:

 var FilePath = Em.Object.extend({
   default: null,
   fileName: Em.computed.oneWay('default'),
   path: Em.computed('fileName', function() {
     return '/path/' + this.get('fileName');
   })
 });

file = FilePath.create() //--> <(unknown mixin):ember279>
file.get('path') //--> /[path]/null
file.set('default', 'untitled.txt') //--> <(unknown mixin):ember279>
file.set('fileName', 'image.jpeg') //--> /[path]/image.jpeg

A usage for such pattern would be to have a property with a default value (note: the sample code has been simplified to expose the issue and does not necessary make sense).

if you run that code with Ember 1.10, it works as expected. However, starting from Ember 1.11 up to the 2.0.3 (I haven't tested after that), you will get the following error when trying set the fileName property:

Assertion Failed: You must use Ember.set() to set the `fileName` property (of <(unknown mixin):ember279>) to `undefined`.

This problem wont occur if you don't get the path first file.get('path'). The fileName property is torn down as expected but the ES5 setter on the fileName property persist. This is the setter who throw the error.

I made two jsfiddle to demonstrate the problem:

@ducarroz ducarroz changed the title cannot alway set a new value to a oneWay property cannot alway set a new value of a oneWay property Feb 12, 2016
@ducarroz ducarroz changed the title cannot alway set a new value of a oneWay property cannot alway set a new value to a oneWay property Feb 12, 2016
@sly7-7
Copy link
Contributor

sly7-7 commented Feb 12, 2016

I've reproduced your example in ember-twiddle, https://ember-twiddle.com/797ef19999d77335cb6f

It turns out that it's not working from ember 1.12 to ember 2.2
It's working since ember 2.3
Not sure if this will be fixed for the bugged versions.

@ducarroz
Copy link
Author

I can reproduce it with ember 1.11 but not in jsfiddle for some reason. Any idea what change was made in 2.3 to fix that problem?

@ducarroz
Copy link
Author

BTW, this issue only exist with debug builds of embers, everything is fine with production builds

@pixelhandler
Copy link
Contributor

@ducarroz @sly7-7 Here is a fork of the twiddle in v1.13.12 https://ember-twiddle.com/bf11f82ea73a37029967?openFiles=application.controller.js%2Capplication.controller.js

My assumption is that there is a workaround for this issue. It's reproducible in some versions and working in the current release. You may get more traction working with a workaround. I'm uncertain about the ability to patch older versions, at least below 1.13.x.

Have you found a workaround yet?

@ducarroz
Copy link
Author

my workaround for now if to patch AliasedProperty_oneWaySet in ember to first delete the potential left over setter before (re)defining the property. Here is the updated method for ember 1.11.4:

  function AliasedProperty_oneWaySet(obj, keyName, value) {
    // cleanup left over setter/getter and cached value
    if (keyName in obj) {
      var m = utils.meta(obj);
      if (m.values && m.values[keyName]) {
        delete m.values[keyName];
      }
      delete obj[keyName];
    }

    properties.defineProperty(obj, keyName, null);
    return property_set.set(obj, keyName, value);
  }

This is kind of brute force, but that does the trick. And again, this is only necessary with the debug version of ember.

I debugged the issue and found out the main difference with ember 2.3, is that the _ember_meta_.values for my oneWay property is actually a descriptor in older version rather than the actual value in ember 2.3. This cause function unwatchKey(obj, keyName, meta) to failed to define the property due to the setter who throw an assert.

@stefanpenner
Copy link
Member

@ducarroz question, does the issue happen in production builds of ember?

[edit]

It appears to work correctly without production builds: http://jsfiddle.net/LthevLpw/10/ (someone should confirm i read the output correctly).

This means, it is most likely related to issues with debug assertions related to mandatory setter, and if that is the case I believe It was fixed by this pr: #12491

Several work-arounds are possible:

  1. If the DK divergence isn't actually wanted (mostly people do want it to change back if the DK changes), then an alternative CP could be created that is used in-place
  2. Mandatory setter can be disabled (also not ideal)
  3. ember-metal/lib/properties.js could be replace at-runtime with a implementation that does the right thing. (also crappy...)

Now the above are not really ideal solutions. Backporting the fix may be possible, but many issues exist in older version with mandatory setter. I am nervous backporting such a large change.

If the options, an alternative CP seems the least invasive.

@ducarroz
Copy link
Author

@stefanpenner, the problem does not occurs in production build because MANDATORY_SETTER_FUNCTION does not have an assert:

// debug
  function MANDATORY_SETTER_FUNCTION(name) {
    return function SETTER_FUNCTION(value) {
      Ember['default'].assert("You must use Ember.set() to set the `" + name + "` property (of " + this + ") to `" + value + "`.", false);
    };
  }

// production
  function MANDATORY_SETTER_FUNCTION(name) {
    return function SETTER_FUNCTION(value) {
          };
  }

But my guess the underlining problem is there, just does not have any apparent consequences

@stefanpenner
Copy link
Member

@ducarroz yes that is what #12491 addressed (among other things).

It is very unlikely that this will be back-ported to 1.11, but we may back-port it to the earliest LTS release that is affected, which that is I am not sure (maybe 1.13?). Maybe @wycats or @tomdale can tell us.

@ducarroz
Copy link
Author

I am seeing a similar issue with a regular computed property:

foo: function() {
  return 'something';
}.property('bar');

I am getting the same assert than for oneWay properties when trying to set its value in test (to fake it). I haven't been able yet to identify the exact circumstances. Wondering if the problem is more wide spread than first thought! I am doing it many time but there is only one instance that trigger the issue.

@stefanpenner
Copy link
Member

the problem is the same, smashing a CP (a CP without a setter) with a value, interferes with the mandatory setter warnings.

@pixelhandler
Copy link
Contributor

@ducarroz Here is another clone of the ember-twiddle example, https://ember-twiddle.com/b87467215eeeea53bf744c2ca29e4889?openFiles=application.controller.js%2Capplication.controller.js using the LTS release. It behaves like the the example you shared for v.1.10.

Since the LTS version has the behavior that works and since v1.13 was supported for much longer than expected, in this case it would be best to use a workaround. It is very unlikely that v1.x will receive any patch releases (Especially since we now have a LTS release, currently 2.4.5)

I'll close this out for now. If you'd like to discuss further perhaps reach out in the embercommunity.slack.com #-help channel to chat regarding the specific version your app is running and potential alternative solutions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants