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

[MAJOR]: rewrite for v3 #379

Merged
merged 77 commits into from
Nov 27, 2019
Merged

[MAJOR]: rewrite for v3 #379

merged 77 commits into from
Nov 27, 2019

Conversation

snewcomer
Copy link
Collaborator

@snewcomer snewcomer commented Nov 19, 2019

Motivation

This major change attempts to solve a variety of (minor?) problems with ember-changeset's more complicated use cases. Notably, working with nested keys and chaining arbitrary nested keys and propagating changes to the UI layer (see this test that was fixed as a result of these changes).

As a result, this necessarily relies on Ember +3.13. with @tracked. No existing top level tests were harmed in this major rewrite. Unused utils + tests were deleted && new utils + tests were added.

Changes

  1. On major pattern changed in this PR is instead of storing keys with arbitrary paths (person.firstName: value) internally on changes/errors, we store it as an object ({ person: { firstName: value } }) without dot notation. This simplifies iterating, chaining and morphing objects and better aligns us with how JS objects work.

  2. Moreover, we don't rely on EmberObject anymore. Certain methods like notifyPropertyChange are imported and used where previously applied. This will allow this library's use outside of Ember.

  3. Because we are using a Proxy, async sets are not possible. We currently rely on this for returning a maybe promise ValidationResult. However, I'm not sure this was ever a necessary API.

  4. We probably shouldn't rely on Ember.set(changeset, ...) anymore. It will set on underlying model if the key is nested. Recommend using changeset.set(...) or for single level keys with native setters, changeset.foo = { key: 'mmbop' };

Will first release a beta release for ppl to try if and only if this PR passes the muster as compared to v2.

close #245 #324 #314 #357
ref #356 #302

@@ -15,7 +15,7 @@ export default function mergeNested<T>(

objects.forEach(obj =>
keys(obj).forEach(key =>
setNestedProperty(finalObj, key, obj[key])
setDeep(finalObj, key, obj[key])
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

replacing set-nested-property with a new implementation set-deep is one of the fundamental differences of v2 vs v3.

// we found some keys!
for (key in kv) {
const val = kv[key];
options.safeSet(target, key, val);
Copy link
Collaborator Author

@snewcomer snewcomer Nov 24, 2019

Choose a reason for hiding this comment

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

Instead of iterating down to leaf nodes, we instead get the path to the leaf value and pass to Ember.set. This helps us work with belongsTo and hasMany relationships (Proxy)

@@ -19,6 +20,7 @@
"inlineSources": true,
"baseUrl": ".",
"module": "es6",
"skipLibCheck": true,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hate to do this, but having a tough time with core libs and only including @glimmer/tracking

@PieterJanVdb
Copy link

Mostly a question to inform myself as I don't know much about native Proxies. Does using Proxy instead of EmberObject for changesets now mean that we can use native setters (user.numbers.cell instead of users.set('numbers.cell')) or is it still recommended you use .set() for everything, as you mentioned in the 4th bullet point of Changes.

From what I can see in the tests you've added some native setters here and there but I guess it's not really recommended we do that everywhere with v3?

@snewcomer
Copy link
Collaborator Author

snewcomer commented Nov 26, 2019

@PieterJanVdb You are right! Single level native setters work. We would have to return a Proxy from the get trap to handle to handle nested key setters (which is probably not a good idea to handle - dont know how deep it is, etc)

54c6ba8

@sandstrom
Copy link

sandstrom commented Nov 28, 2019

@snewcomer Awesome! 🏅

Does this change how relationships are handled?

More specifically, can v3 handle things such as this:

  • Create a model m, e.g. an instance of Mother (Ember Data instance)
  • Attach two children Child to the mother
  • Rollback and have the children removed

@snewcomer
Copy link
Collaborator Author

@sandstrom 👋 I have a test for rolling back a hasMany relationship. Let me know if this does not work though. With these new changes, it shouldn't be too hard to figure out if it doesn't work.

@sandstrom
Copy link

@snewcomer Awesome, will take a look!

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.

RFC: Use es6 proxy
5 participants