Skip to content

Conversation

@ohcibi
Copy link
Contributor

@ohcibi ohcibi commented Mar 19, 2020

Stringifying and parsing again will throw when obj has a circular
structure which can exist when e.g. ember data relationships are values
in the changesets underlying object. Fixes #19

Stringifying and parsing again will throw when `obj` has a circular
structure which can exist when e.g. ember data relationships are values
in the changesets underlying object. Fixes adopted-ember-addons#19
@snewcomer
Copy link
Collaborator

Just putting down my thoughts. This might be the right way to go.

At one point, I remember I needed to avoid some consumer mutating it so I wanted to return a unique object every time. Do you think there might be an alternate solution as well?

@ohcibi
Copy link
Contributor Author

ohcibi commented Mar 20, 2020

@snewcomer 🤔 so the point was to create a copied object of the value to avoid mutation? Well I kind of like the idea but the thing is I didn't even expected that value reference to be immutable and its also not (yet) mentioned in the readme. On the other hand I'm unsure if we actually want to copy that value on every .get('error').

Regarding an alternative solution, I could imagine having something like a lightweight proxy that basically makes the value properties read only, i.e. a proxy that does nothing on .set and returns a proxied value for .get, such that nested structures are readonly in deeper levels as well. Roughly:

const handler = {
  get(obj, prop) {
    return typeof obj[prop] === 'object' ? new Proxy(obj[prop], handler) : obj[prop];
  },
  set() {}
};

and then in addError:

const value = new Proxy(theValue, handler);

I'd suggest to address that topic in a new issue though. Currently the visible and inevitable problem is that because of JSON.stringify one cannot use values with circular structures, which are not uncommon when using ember-data, as values in a changeset. Making the value immutable should be well thought but this issue right here breaks using the plugin with ember-data and relations which we should address fast imo.

@snewcomer
Copy link
Collaborator

@ohcibi There is one thing we need to do before we merge as it will break tests in ember-changeset - convert it from the Err{} class to a plain pojo...

@snewcomer snewcomer merged commit d5ff4de into adopted-ember-addons:master Mar 21, 2020
@snewcomer
Copy link
Collaborator

We will just update ember-changest to be compatible. Thanks for the PR!

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.

Error validating async BelongsTo relationship

2 participants