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

Don't stringify and parse the error object to and from json #22

Merged
merged 1 commit into from
Mar 21, 2020

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