'getting' a Moment instance on a changeset returns a relay instead of that instance

[PieterJanVdb]

When having a Moment instance on a changeset, eg.: set(changeset, 'startDate', moment()), and if you then later do get(changeset, 'startDate') it will - instead of returning the Moment instance on the changeset - return a relay with 'startDate' as key.

I'm unsure if this is a 100% a bug, as the Moment instance is an object so ember-changeset thinks it has to create a relay, but it would be nice if there were a way to deal with this (eg. defining keys for which no relay should be created, as I'm not going to directly get or modify the internals of the Moment instance).

@nucleartide

[nucleartide]

Thanks for reporting! This is similar to a1d22b7, I'd consider this a bug.

Will include this in the upcoming bugfix release (1.4.2).

[nucleartide]

As mentioned on Slack, for now you could try changeset.get('changeset.someMomentDate'), since the Relay object stores a reference to changeset.

[nucleartide]

@PieterJanVdb The latest release v1.4.2-beta.0 fixes this. You can write changeset.get('someMomentDate') now and it'll return a Moment instance instead of a Relay.

If you have the time, please try it out (with caution :D). It should still work with changeset.get('changeset.someMomentDate') as well.

(Official announcement post is over on Ember Way).

[jelhan]

I don't think this is fixed. Here is a reproduction for v1.5.0-beta.0: https://ember-twiddle.com/f38bf913c6c44079ba0c84c148cc9f05?openFiles=templates.application.hbs%2C It's the same if you downgrade ember-changeset to 1.4.2-beta.0.

If accessing the moment object throw changeset (e.g. using moment-format helper as in twiddle) this is blowing up due to .get() assertion:

Assertion Failed: You attempted to access the `_isAMomentObject` property (of <(unknown):ember216>).
Since Ember 3.1, this is usually fine as you no longer need to use `.get()`
to access computed properties. However, in this case, the object in question
is a special kind of Ember object (a proxy). Therefore, it is still necessary
to use `.get('_isAMomentObject')` in this case.

Please note that this assertion is triggered by moment.js code.

The regression has been added in 1.4.0. It's working fine for 1.3.0.

[drummy2]

This issue needs re-opening, it seems it has made it's way into the latest release!

[snewcomer]

Agreed. I'll find time this week to put up a fix if possible. After that is fixed, let's release 1.5 proper!

[snewcomer]

@PieterJanVdb @drummy2 @jelhan 👋 Could somebody look at this test I added in #309 and lmk how it diverges from this issue? I am asserting that "getting" startDate returns the Moment class in the test below. So I must be missing something. 🙇

https://github.com/poteto/ember-changeset/blob/f4017ee4ee5d9b22fb431de5d32434f49a8f9dc9/tests/unit/changeset-test.js#L340

[jelhan]

@snewcomer Your test is fine. While trying to debug I noticed that the bug only occurs if changeset template helper is used. Here is another ember-twiddle demonstrating that it's working fine if changeset is constructed in code: https://ember-twiddle.com/eeb4e12e66b114c12e7204b218d359a2?openFiles=templates.application.hbs%2C

This is the ember twiddle showing that it's not working if template helper is used: https://ember-twiddle.com/f38bf913c6c44079ba0c84c148cc9f05?openFiles=templates.application.hbs%2C

---

Debugged further and noticed that the bug is only occuring if ember getter is used and the value is proxied to content:

class Moment {}

let c = new Changeset({
  foo: new Moment()
});
c.set('bar', new Moment());
    
console.log(c.get('foo') instanceof Moment); // true
console.log(get(c, 'foo') instanceof Moment); // false
console.log(c.get('bar') instanceof Moment); // true
console.log(get(c, 'bar') instanceof Moment); // true

Here is an ember-twiddle: https://ember-twiddle.com/a5ea906707f67f978454d0164b1da8ed?openFiles=controllers.application.js%2C

This is not covered by your test since you are only testing getter after setting the value.

[snewcomer]

@jelhan I'm a little stumped at the moment. We have existing tests that say using Ember.get will not give you the actual content and that if using Ember.get, you should get via .content.

The issue is that foo is set on the CONTENT of the changeset. We return the relay instance from the changesets unknownProperty method so that we can work with nested keys when CONTENT exists on the changeset. Otherwise we return the original value.

Not sure of a solution yet...

[jelhan]

As far as I got it, it's an issue by design. If the original value is an object, a relay is returned: https://github.com/poteto/ember-changeset/blob/v1.5.0-beta.2/addon/index.js#L769-L774 The value is considered an object if ember's typeOf utility considers it as an object or instance. (https://github.com/poteto/ember-changeset/blob/v1.5.0-beta.2/addon/utils/is-object.js) That is true except for some special types like Array, Date, String, Number. So this issue is not only related to moment but to every value that is represented by an object except for some whitelisted types. Moment is just the most widely used example.

Is there a good reason for returning a relay for an object which isn't an instance of EmberObject nor a POJO? This would fix the issue for all objects representing a value but on the other hand still allow to work with nested keys on EmberObject's and POJO's.

An implementation would look like this:

// /addon/utils/is-object.js
import { typeOf } from '@ember/utils';

export default function isObject(val) {
  return
    // instances of EmberObject
    typeOf(val) === 'instance' ||
    // POJO
    Object.getPrototypeOf(val) === Object.prototype;
}

[snewcomer]

@jelhan Hm that may work. So my only question is what if val is new Dog() or something like that? Also, typeOf returns the correct types if I'm not mistaken.

e.g.
Ember.typeOf([]) === "array"

[jelhan]

So my only question is what if val is new Dog() or something like that?

It depends:

• class Dog extends EmberObject{}: Will return a relay cause Dog extends EmberObject and therefore typeof(new Dog()) === 'instance' is true.
• function Dog() {}: Will return instance of Dog cause Object.getPrototypeOf(val) === Object.prototype is false (and it's not an instance of EmberObject).
• function Dog() {}; Dog.prototype = Object.prototype: Will return a relay cause Object.getPrototypeOf(val) === Object.prototypeistrue` (but I hope nobody is doing something like that).

Also, typeOf returns the correct types if I'm not mistaken. e.g. Ember.typeOf([]) === "array"

It depends what you consider "the correct type". I'm arguing that type "object" of ember's typeOf function is to broad for our use case:

Ember.typeOf({}) === 'object';
Ember.typeOf(moment()) === 'object';
Ember.typeOf(new Dog()) === 'object';

[snewcomer]

Yeah my concern is if the content is an instance of class Dog.

What we ultimately want is something like this line. If it is a relay, return the content of the relay like we do when overriding the changeset.get.
https://github.com/poteto/ember-changeset/blob/master/addon/index.js#L861

[snewcomer]

I'm starting to wonder if we need to rethink or rejigger how relays interact with changesets. I'm not sure the limitation of only using changeset.get is acceptable. Also relays provide a very powerful nested key implementation, but using get(... is one big headache

[snewcomer]

Alrighty the template helper has been fixed. Should not return a Relay instance anymore!

[snewcomer]

Going to close for now! Lmk if anybody has any questions!

[pgengler]

I'm still having this issue (or possibly a similar-and-related issue, since mine is happening in the JS for a component and not in a template).

I put together a new app with enough code to reproduce the problem: https://github.com/pgengler/ember-changeset-issue/tree/problems-with-moment

To reproduce

• check out the problems-with-moment branch of that repo
• yarn install
• ember s
• Visit http://localhost:4200/recipes/1
• Note the failing assertion in the JS console

[jelhan]

Alrighty the template helper has been fixed. Should not return a Relay instance anymore!

What scenarios should be considered fixed? Using a moment instance stored in a changeset with moment helpers still throws using 1.5.0. This was the scenario in my first ember twiddle. You tried to update the version of ember-changeset but it still throws.

Please note that this is a regression added 1.4.x. The twiddle is working fine with 1.3.0.

[snewcomer]

@jelhan See these two tests.

Note in your example, you have date=(moment). Removing the parens gets it to pass with 1.5.0. I'm not exactly sure what the parens is doing. In JS, I would imagine it would be ignored. But in the template it is causing issue.

[jelhan]

See these two tests.
The tests does not address the issue shown in the twiddle. I'm not quite sure what they are for, but they do not test that there aren't any relays involved in template.

Let me give some more insight what these ember twiddle does:

• It constructs a new changeset with a hash that contains a moment instance constructed by moment helper, which is provided by ember-moment.
• It passes the moment instance on changeset to log helper to demonstrate that a Relay is returned.
• It passes the moment instance on changeset to moment-format to demonstrate that this is a real-world issue.

moment-format is another template helper provided by ember-moment. It could be used to format a moment instance. In that process code of moment.js is involved which access properties of that object, which isn't a surprise at all. As moment.js does not care about ember.js it does not use get() method to access properties on it's own instance, which isn't a surprise either. But since we haven't passed a moment instance but a relay of a moment instance to moment-format it throws:

Assertion Failed: You attempted to access the `_isAMomentObject` property (of <(unknown):ember213>).
Since Ember 3.1, this is usually fine as you no longer need to use `.get()`
to access computed properties. However, in this case, the object in question
is a special kind of Ember object (a proxy). Therefore, it is still necessary
to use `.get('_isAMomentObject')` in this case.

If you encountered this error because of third-party code that you don't control,
there is more information at https://github.com/emberjs/ember.js/issues/16148, and
you can help us improve this error message by telling us more about what happened in
this situation.

The only work-a-round is to access the underlying object through .content property of the relay. But this is not a good developer experience at all.

Maybe I didn't got the point, but what benefits do we have from using a relay for complex objects like moment at all? Or to put it another way around: Why do we use a relay for a moment instance but not for Date, Array, Number? As suggested before a relay should only be used for POJOs and instances of EmberObject. Maybe a user should be able to whitelist additional classes, which should support deep set (and therefore use a relay), in configuration.

Note in your example, you have date=(moment). Removing the parens gets it to pass with 1.5.0. I'm not exactly sure what the parens is doing. In JS, I would imagine it would be ignored. But in the template it is causing issue.

The parens are causing moment to be treated as a helper not as a variable. So moment here refers to the helper provided by ember-moment. Without parens moment is treated as a variable. Cause there isn't any with that name, it's undefined. So in that case you don't have any instance of moment and therefore the bug is not triggered. If you assign a moment instance to that variable in application controller, you will see the bug again.

[snewcomer]

@jelhan I see. Yep the parens are needed 👍 thx. I was mistaken. Accessing the date key on the changeset is the same issue b/c it uses get(... internally.

Truthfully there are use cases for which the Relay are not solved for (because of which I will be testing out just plain Proxy's after October).

You are 100% right that we could scope this to only POJOs and EmberObjects. I think at a minimum, putting up a PR for now would be helpful. I just don't know what all the uses are out there in the wild for ember-changeset. Me personally - I just use them for EmberObjects.

[sevab]

Not sure if this is related to this issue:

I run a custom validation (via changeset-validations addon) on a changeset property with a moment instance.

I then pass the changeset value to ember-power-calendar with selected=(hash start=changeset.startDate.content end=changeset.endDate.content) and handle date changes in the actions:

setMomentDate(momentRange) {
  let { start, end } = momentRange;
  this.get('changeset').setProperties({
    startDate: start,
    endDate: end
  });
}

As I change the dates and validation passes, changeset.momentDate returns a relay object and the moment instance is accessible via changeset.momentDate.content in the template – everything works as expected.

However, when validation fails, changeset sets moment instance directly on changeset.momentDate (not wrapped in a relay object), so changeset.momentDate.content returns undefined and calendar is unable to read the dates.

Not sure if this is expected behaviour of ember-changeset or if something's possibly wrong on my end.

This is on latest ember-changeset & ember-changeset-validations, ember 2.16.2.

[snewcomer]

Just to notify everyone, 2.0.0-beta.0 has been published for ember-changeset and ember-changeset-validations. Give it a shot! @sevab i think it might fix your issue but don't quote me on it :)

[sevab]

@snewcomer all seems to works with 2.0, thanks!

Though In handlebars had to switch
selected=(hash start=changeset.startDate.content end=changeset.endDate.content)
to selected=(hash start=changeset.startDate end=changeset.endDate).

Which is a changed behaviour, but more convenient.

[Duder-onomy]

Just ran across this issue, identical problem to what was described by most (changeset + moment + ember-power-calendar) Version 2.0.0 everything works. Thanks @snewcomer! You rawk.

[bterkuile]

Got a similar problem, setting a moment() date on a changeset and then getting it back does not return the Moment object.
setup:

• ember-changeset 3.3.1
• ember-source 3.18.0
• ember-cli-moment-shim

After tracking all the callbacks the pivot point I found was in ember-changeset/addon/index.js in the get method (line 150).

My temporary hack to make it work is to trick the get method into thinking that a Moment object is a belongsTo relationship (better to have a specific or generalized implementation for this):

now = moment()
now.constructor.prototype.isLoading = true
now.constructor.prototype.isLoaded = true
now.constructor.prototype.isNew = true
now.constructor.prototype.hasDirtyAttributes = true

hopefully this saves somebody the time!

[snewcomer]

@bterkuile 3.3.2 should fix this!

[bterkuile]

Thank you @snewcomer, the solution looks nice. I guess the strategy is that the netKeys.length will be zero. In my test the moment object does not to appear to be so 'clean'. I got:

netKeys == ["_f", "_isUTC", "type"]
# or other situation just
netKeys == ["_isUTC"]

My understanding of what exactly is happening is low, so don't know where the cause of this is.

Maybe the Ember get strategy should allow an object to have a property specifying it is an 'end leaf' and throw an error when a hackery user (like me) tries to set a computed property on a property if that object, but probably has some unforeseen consequences.

[snewcomer]

@bterkuile Can you see a failing test that captures your case? I'm having trouble viewing your use case.

https://github.com/poteto/ember-changeset/pull/465/files#diff-f721c38c4110379388cab513565fec56R565

Honestly I have not been a fan of this block. I was leary about it when it first went into master but could not think of an alternative solution.

I'm throwing around the idea of a registerModel class and we could use it in a way to prevent collapsing a class down to an object. But first, if we could get a failing test, that would be wonderful!

[bterkuile]

@snewcomer I'll give it a shot, it will take some digging in finding out how these moment attributes are created

[bterkuile]

To create the failing test:

Step 1.

Check keys on result in-stead of content[baseKey] (addon/index.js):

          let netKeys = Object.keys(result).filter(k => !this.safeGet(result, k));

Step 2

Add a dynamic attribute having a non truthy value:

  test('#set adds a change if value is an object with existing value', async function(assert) {
    class Moment {
      constructor(date) {
        this.date = date;
      }
    }

    let d = new Date();
    dummyModel.set('startDate', new Moment(d));
    dummyModel.set('name', 'Bobby');

    let c = Changeset(dummyModel);
    d = new Date();
    let momentInstance = new Moment(d);
    momentInstance._isUTC = false;
    c.set('startDate', momentInstance);

    let expectedChanges = [{ key: 'startDate', value: momentInstance }];
    let changes = get(c, 'changes');

    assert.deepEqual(changes, expectedChanges, 'should add change');

    let newValue = c.get('startDate');
    assert.deepEqual(newValue, momentInstance, 'correct getter');
    assert.ok(newValue instanceof Moment, 'correct instance');
    assert.equal(newValue.date, d, 'correct date on moment object');

    newValue = get(c, 'startDate');
    assert.deepEqual(newValue, momentInstance, 'correct getter');
    assert.ok(newValue instanceof Moment, 'correct instance');
    assert.equal(newValue.date, d, 'correct date on moment object');
  });

git diff

--- a/addon/index.js
+++ b/addon/index.js
@@ -155,7 +155,7 @@ export class EmberChangeset extends BufferedChangeset {
           isObject(content[baseKey]) &&
           !isBelongsToRelationship(content[baseKey])
         ) {
-          let netKeys = Object.keys(content[baseKey]).filter(k => !this.safeGet(result, k))
+          let netKeys = Object.keys(result).filter(k => !this.safeGet(result, k));
           if (netKeys.length === 0) {
             return result;
           }
diff --git a/tests/unit/changeset-test.js b/tests/unit/changeset-test.js
index b197302..488ed1a 100644
--- a/tests/unit/changeset-test.js
+++ b/tests/unit/changeset-test.js
@@ -568,6 +568,7 @@ module('Unit | Utility | changeset', function(hooks) {
     let c = Changeset(dummyModel);
     d = new Date();
     let momentInstance = new Moment(d);
+    momentInstance._isUTC = false;
     c.set('startDate', momentInstance);
 
     let expectedChanges = [{ key: 'startDate', value: momentInstance }];

suggestions

It's hard to do suggestions since I am not into this code, but I can make a git diff suggestion doing a special check on the object, allowing creators of the objects themselves to opt-out of the 'nesting' of the object.

[bterkuile]

Got an idea working that allows objects to behave as 'leaf' objects. Don't know if it fits and some other specs are failing with these changes.

git diff --cached

diff --git a/addon/index.js b/addon/index.js
index f57fe4d..3d636cf 100644
--- a/addon/index.js
+++ b/addon/index.js
@@ -3,6 +3,7 @@ import { BufferedChangeset, normalizeObject, pureAssign } from 'validated-change
 import mergeDeep from './utils/merge-deep';
 import isObject from './utils/is-object';
 import { isBelongsToRelationship } from './utils/is-belongs-to-relationship';
+import { isLeafObject } from './utils/is-leaf-object';
 import { notifyPropertyChange } from '@ember/object';
 import { tracked } from '@glimmer/tracking';
 import { get as safeGet, set as safeSet } from '@ember/object';
@@ -153,9 +154,10 @@ export class EmberChangeset extends BufferedChangeset {
         if (
           !Array.isArray(result) &&
           isObject(content[baseKey]) &&
-          !isBelongsToRelationship(content[baseKey])
+          !isBelongsToRelationship(content[baseKey]) &&
+          !isLeafObject(result)
         ) {
-          let netKeys = Object.keys(content[baseKey]).filter(k => !this.safeGet(result, k))
+          let netKeys = Object.keys(result).filter(k => !this.safeGet(result, k));
           if (netKeys.length === 0) {
             return result;
           }
diff --git a/addon/utils/is-leaf-object.js b/addon/utils/is-leaf-object.js
new file mode 100644
index 0000000..dc0f03c
--- /dev/null
+++ b/addon/utils/is-leaf-object.js
@@ -0,0 +1,18 @@
+// Check of the object is to be considered a leaf in a nested structure
+// and therefore no further proxy wrapped
+export function isLeafObject(obj) {
+  if (!obj) {
+    return true;
+  }
+
+  if (obj._isAMomentObject) {
+    return true;
+  }
+
+  if (obj._isLeafObject) {
+    return true;
+  }
+
+  return false;
+}
+
diff --git a/tests/unit/changeset-test.js b/tests/unit/changeset-test.js
index b197302..3909d18 100644
--- a/tests/unit/changeset-test.js
+++ b/tests/unit/changeset-test.js
@@ -556,6 +556,7 @@ module('Unit | Utility | changeset', function(hooks) {
 
   test('#set adds a change if value is an object with existing value', async function(assert) {
     class Moment {
+      _isAMomentObject = true;
       constructor(date) {
         this.date = date;
       }
@@ -568,6 +569,7 @@ module('Unit | Utility | changeset', function(hooks) {
     let c = Changeset(dummyModel);
     d = new Date();
     let momentInstance = new Moment(d);
+    momentInstance._isUTC = false;
     c.set('startDate', momentInstance);
 
     let expectedChanges = [{ key: 'startDate', value: momentInstance }];

[snewcomer]

@bterkuile Thank you! I see the general problem and it is a tough problem. We can handle set obj.name, but if you get obj, then we need to merge the original content - e.g. email in obj as well. If we don't, then the c.get('obj') might be missing some keys at the template layer. However, if you set c.set('obj', newObj) with a brand new value, we still are merging the original keys when you get obj. Both of these scenarios have bugs...

Effectively, we need to re-create that object when you get it, with its updated value and sibling keys...

Will try to think through a solution. Reopening and thank you for sticking through with this!

[bterkuile]

@snewcomer thank you for thinking along with this, I guess it's a complex yet interesting problem connected with the changeset way of handling data. I guess especially starting with a null/undefined value on the original object and setting it to a value or the other way around.

[snewcomer]

Ideally we flip the solution. Instead of keeping track of the CHANGES and merging the CONTENT sibling keys, we return CONTENT with the specific key that has changed applied to it. That would require a deep copy (not shallow copy) and need to avoid data loss. Not sure bringing Lodash's deepClone and some sort of immutable construct would not be backwards compatible. But we certainly don't want to mutate the existing CONTENT (original obj passed to Changeset).

We are right in the crosshairs of structured cloning, but not something we can do at this time (ever?) and seems like abusing an API.

My feeling is that we have to live with shallow clones if we have to merge sibling keys to/or from CHANGES. If we return from get, then we expect it to be read only. If you mutate the object (and not the changeset itself), well that is on you (tisk tisk).

I'll explore this today...