[WIP] Rewrite to use native Proxy #627

BryanCrotaz · 2021-12-07T16:59:33Z

Work in progress. Using existing test suite plus some more edge cases and preserving public API.

Enables developer to use normal JS semantics:

var output = changeset.gruffalos.lastObject.say('grrr');
changeset.people[1].name.first = 'Bryan';

Really useful for having nested components:

{{#let (changeset this.model) as |changeset|}}
  {{#each changeset.people as | person |}}
    <PersonForm @model={{person}}/>
  {{/each}}
{{/let}}

PersonForm doesn't have to know that this is a changeset. The save and rollback can be at a higher level.

NullVoxPopuli · 2021-12-07T17:11:07Z

addon/classes/ChangesetArrayProxyHandler.ts

+export default class ChangesetArrayProxyHandler<T extends Input> implements IChangesetProxyHandler<T> {
+  constructor(source: T, _options?: ProxyOptions) {
+    if (source?.constructor?.name === 'ArrayProxy') {
+      this.__sourceProxy = source as ArrayProxy<any>;


these should either by symbol properties, or private properties, as they may conflict with folks data

you mean __sourceProxy etc?

I don't think these do conflict. Proxy is not a wrapper that falls back to the internals like Ember.ObjectProxy. It explicitly calls in with get and set and you do your own fallback. __sourceProxy is private api so it's not available through get and set, and if there was a name conflict it would fall through to the proxied object correctly.

NullVoxPopuli · 2021-12-07T17:12:17Z

addon/classes/ChangesetArrayProxyHandler.ts

+
+  public get(_target: T, key: string): any {
+    // extra stuff
+    switch (key) {


you can shorten this by doing if (typeof this.readArray[key] === 'function') return this.readArray[key];, etc

If you notice I have two different ways of doing it
readArray[key] returns the proxy or the source if there's no changes
writeArray[key] creates a proxy if it doesn't exist and returns it

I thought about using a map to choose which one to return. Otherwise readKeys.includes[key] etc

Thoughts?

typeof this.readArray[key] === 'function' doesn't work - typescript expects the index to be a number. Is there a way to iterate methods on an array without [key]?

I aw not sure

NullVoxPopuli · 2021-12-07T17:12:40Z

addon/classes/ChangesetArrayProxyHandler.ts

+  @action
+  public getValue(key: string | symbol) {
+    switch (key) {
+      // backwards compatible


yay back compat

NullVoxPopuli · 2021-12-07T17:13:33Z

addon/utils/is-unchanged.ts

@@ -0,0 +1,7 @@
+export default function isUnchanged(a: any, b: any) {


I'm loving all these utilities

NullVoxPopuli · 2021-12-07T17:14:45Z

So, I like where this is going, but the real implementation is here: https://github.com/validated-changeset/validated-changeset
is there a reason you opted to only update this library? or is there a PR I'm missing? 🤔

BryanCrotaz · 2021-12-07T17:31:56Z

I didn't realise. But looking at it, validated-changeset is supposed to be independent of Ember. So it shouldn't use Tracking for example. I don't think it's possible to pull tracking out of my design. I was aiming for a really clean and simple design that gets rid of a ton of cruft.

NullVoxPopuli · 2021-12-07T17:45:22Z

validated-changeset is supposed to be independent of Ember

is true -- but it's also where most of the complexity comes in.

is the goal to eliminate the need for validated-changeset?

and fwiw, I think it is possible to a better native proxy design with validated-changeset and provide deep reactivity as well -- but, I want to make sure I know what your goals are?

BryanCrotaz · 2021-12-07T18:32:14Z

My goals are to have ember-changes et working for deep nesting without get and set. If that means submitting to validated- changeset instead then that's fine by me

NullVoxPopuli · 2021-12-07T18:35:56Z

Ah ok, we can probably do even less work then. Thanks for clarifying!

NullVoxPopuli · 2021-12-16T11:29:08Z

addon/classes/ChangesetArrayProxyHandler.ts

+  }
+
+  public get isValid(): boolean {
+    // TODO: validate this level


NullVoxPopuli · 2021-12-16T11:30:17Z

addon/classes/ChangesetArrayProxyHandler.ts

+    // TODO: validate this level
+
+    // now look at all the nested proxies
+    for (var proxy of this.__nestedProxies.values()) {


Why reimplement these behaviors? Couldn't you return tte value from the underlying validated-changeset?

NullVoxPopuli · 2021-12-16T11:32:58Z

addon/classes/ChangesetObjectProxyHandler.ts

+        throw `changeset.${key} is readonly`;
+    }
+    // nested keys are separated by dots
+    var [localKey, subkey] = splitKey(key as string);


What about deeply nested keys? Maybe 5+ layers deep?

NullVoxPopuli · 2021-12-16T11:34:48Z

addon/classes/ChangesetObjectProxyHandler.ts

+    // have to use get() here because source might be an EmberProxy
+    const oldValue = get(this.__source, localKey);
+    const unchanged = isUnchanged(value, oldValue);
+    let changes = this.__changes;


Wait, does this PR remove and re-implement validated-changeset?

Sorry if obvious

BryanCrotaz · 2021-12-17T02:40:51Z

closing in favour of adopted-ember-addons/validated-changeset#150

BryanCrotaz added 4 commits December 6, 2021 14:37

fix linting

c5b370d

change with to let helper

a42ecce

a start

ae3c407

works with nested object and array keys

2eafbd0

NullVoxPopuli reviewed Dec 7, 2021

View reviewed changes

addon/utils/is-unchanged.ts

@@ -0,0 +1,7 @@

export default function isUnchanged(a: any, b: any) {

Copy link

NullVoxPopuli Dec 7, 2021

Choose a reason for hiding this comment

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

I'm loving all these utilities

BryanCrotaz added 3 commits December 7, 2021 17:37

handle object or array at the top level

9c4b1e5

clean up

7f0315f

clean up

990189a

NullVoxPopuli reviewed Dec 16, 2021

View reviewed changes

addon/classes/ChangesetArrayProxyHandler.ts

}

public get isValid(): boolean {

// TODO: validate this level

Copy link

NullVoxPopuli Dec 16, 2021

Choose a reason for hiding this comment

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

Todo!

NullVoxPopuli reviewed Dec 16, 2021

View reviewed changes

BryanCrotaz closed this Dec 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Rewrite to use native Proxy #627

[WIP] Rewrite to use native Proxy #627

BryanCrotaz commented Dec 7, 2021 •

edited

Loading

NullVoxPopuli Dec 7, 2021

BryanCrotaz Dec 7, 2021

BryanCrotaz Dec 7, 2021

NullVoxPopuli Dec 7, 2021

BryanCrotaz Dec 7, 2021

BryanCrotaz Dec 8, 2021

NullVoxPopuli Dec 16, 2021

NullVoxPopuli Dec 7, 2021

NullVoxPopuli Dec 7, 2021

NullVoxPopuli commented Dec 7, 2021

BryanCrotaz commented Dec 7, 2021

NullVoxPopuli commented Dec 7, 2021 •

edited

Loading

BryanCrotaz commented Dec 7, 2021

NullVoxPopuli commented Dec 7, 2021 •

edited

Loading

NullVoxPopuli Dec 16, 2021

NullVoxPopuli Dec 16, 2021

NullVoxPopuli Dec 16, 2021

NullVoxPopuli Dec 16, 2021

BryanCrotaz commented Dec 17, 2021

		@@ -0,0 +1,7 @@
		export default function isUnchanged(a: any, b: any) {

[WIP] Rewrite to use native Proxy #627

[WIP] Rewrite to use native Proxy #627

Conversation

BryanCrotaz commented Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli commented Dec 7, 2021

BryanCrotaz commented Dec 7, 2021

NullVoxPopuli commented Dec 7, 2021 • edited Loading

BryanCrotaz commented Dec 7, 2021

NullVoxPopuli commented Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BryanCrotaz commented Dec 17, 2021

BryanCrotaz commented Dec 7, 2021 •

edited

Loading

NullVoxPopuli commented Dec 7, 2021 •

edited

Loading

NullVoxPopuli commented Dec 7, 2021 •

edited

Loading