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

Callback triggered for changes on inheriting objects #74

Closed
swar30 opened this issue Feb 14, 2021 · 11 comments
Closed

Callback triggered for changes on inheriting objects #74

swar30 opened this issue Feb 14, 2021 · 11 comments

Comments

@swar30
Copy link

swar30 commented Feb 14, 2021

I see some unexpected behavior and was wondering if may be I'm streching beyond on-change intended use case.

Lets I have an objects (lets call it obj) for which I created a proxy using on-change.

Now I have a flow that creates new object (lets call it derived) using the proxy as prototype.

When I set some property on obj I expect callback to be called and indeed it is.

When I set some new property on derived that does not exist on obj, here again callback is called.
Since I don't modify watched object, but rather object that has watched object as prototype, I expect that callback won't be called, but it is.

From what I can see, on-change does not take this scenario into account or may be that is by design ?

Would love to hear your opinion on this.

P.S. What I'm trying to do is to protect objects from been modified, but still allow objects that have them as prototypes be modified to mask values on prototype.

Thanks.

@DarrenPaulWright
Copy link
Collaborator

Would you mind providing a simple working code sample? Thanks!

@swar30
Copy link
Author

swar30 commented Feb 21, 2021

Sure thing, will prepare and post here in next few days.

@swar30
Copy link
Author

swar30 commented Feb 21, 2021

@DarrenPaulWright please see https://codepen.io/alex-shnayder/pen/rNWGBQL
While writing the examples I noticed that not only callback is triggered but also that the original subject object is modified when I expect it to not be modified at all.

In the examples I have both plain JS example and one using on-change.
Hope it's clear, if not let me know and I will fix it as needed.

Thanks.

@swar30
Copy link
Author

swar30 commented Mar 4, 2021

@DarrenPaulWright please see https://codepen.io/alex-shnayder/pen/rNWGBQL
While writing the examples I noticed that not only callback is triggered but also that the original subject object is modified when I expect it to not be modified at all.

In the examples I have both plain JS example and one using on-change.
Hope it's clear, if not let me know and I will fix it as needed.

Thanks.

@DarrenPaulWright did you have a chance to look at my example.

I would like to hear yuour feedback, and understand if may be it will make sense to adjust on-change to support this scenario.
I would be happy to create PR to change this behavior, just want to be sure what you would expect to happen in this case and which changes will be welcome.

Thanks.

@DarrenPaulWright
Copy link
Collaborator

Sorry if I'm missing something, but could you just do Object.create(subject)?

@swar30
Copy link
Author

swar30 commented Apr 7, 2021

@DarrenPaulWright what do you mean by do Object.create do you mean instead of using on-change,
Or when I'm using on-change ?

@DarrenPaulWright
Copy link
Collaborator

In your code you are using const derivedObject = Object.create(watchedSubject);. If you don't want to see callbacks with changes to the prototype of derivedObject, then why not do const derivedObject = Object.create(subject);?

As far as I can tell, there is no way for a Proxy to detect when it gets added as a prototype to another object or to detect later if it is part of a prototype chain.

Another option if you must use watchedSubject for the prototype is, inside the onChange callback, use Object.prototype.isPrototypeOf() to ignore changes.

@swar30
Copy link
Author

swar30 commented Apr 11, 2021

@DarrenPaulWright I tried to not deep dive into details of what I'm doing to make it simpler, but may be that actually made it more complicated to understand the use case.

We have some objects which we are trying to protect from modification during development, today we are using Object.freeze to do deep freeze of objects.

If you freeze an object and try to modify any of it's properties it will fail.
But if you do const newObj = Object.create(obj) and try to modifify newObj properties that is ok,
because original obj is not changed and only new props are created on newObj.

The problem with using Object.freeze is that when you have large objects with deep nest level that becomes an expensive operation with real impact on application performance.

I was hoping to use on-change to solve same problem of preventing modification to objects while at same not paying same performance price up front.

So back to your question original question, skipping watchedSubject is not an option, as consumer not supposed to be aware at all what they are working.

I will look at your suggestion of using check inside onChange to detect when operation is applied to derived or original object, not sure how that will work with deep nesting, but I will give it a try.

As for not been able to detect inside Proxy when operation is performed on watchedSubject or on derived object.
I will give it some thought and will get back to you if I come up with any thing.

Thanks.

@swar30
Copy link
Author

swar30 commented Apr 12, 2021

Another option if you must use watchedSubject for the prototype is, inside the onChange callback, use Object.prototype.isPrototypeOf() to ignore changes.

Checked and not possible, because this passed into onChange callback is the original subject and not the object on which operation is applied.

Here

set(target, property, value, receiver) {
    value = getProxyTarget(value);

    const reflectTarget = target[proxyTarget] || target;
    const previous = reflectTarget[property];
    const hasProperty = property in target;

    if (cache.setProperty(reflectTarget, property, value, receiver, previous)) {
        if (!equals(previous, value) || !hasProperty) {
            handleChangeOnTarget(target, property, previous, value);
        }

        return true;
    }

    return false;
},

reciever is the object which I'm actually intrested in, if I had that I could have performed check to see if operation is performed on original or derived object, but it's not passed in into handleChangeOnTarget.

Does it make sense to add receiver as param to handleChangeOnTarget for it to propogate into onChange callback ?

@swar30
Copy link
Author

swar30 commented Apr 14, 2021

I think I will just go with custom Proxy based solution, while it would have been nice to use on-change I think it might be a bit much to start doing changes to it just for this use case.

I will link here to my code sample once I'm done

@swar30 swar30 closed this as completed Apr 14, 2021
@swar30
Copy link
Author

swar30 commented Apr 18, 2021

In case any one will be intrested in the final solution I'm going with

/**
 * Utility for making objects read only using Proxy.
 * Instead of using Object.freeze which is expensive at runtime as it will eagerly freeze all props.
 * An operation which can take long time for large and deep nested objects.
 */

type ImmutablePrimitive = undefined | null | boolean | string | number | Function;
type ImmutableMap<K, V> = ReadonlyMap<Immutable<K>, Immutable<V>>;
type ImmutableSet<T> = ReadonlySet<Immutable<T>>;
type ImmutableObject<T> = {readonly [K in keyof T]: Immutable<T[K]>};

export type Immutable<T> = T extends ImmutablePrimitive
	? T
	: T extends Map<infer K, infer V>
	? ImmutableMap<K, V>
	: T extends Set<infer M>
	? ImmutableSet<M>
	: ImmutableObject<T>;

class ReadonlyObjectModification extends TypeError {
	constructor(message: string, public target: object) {
		super(message);
	}
}

const objectToProxyMap = new WeakMap<object, ProxyHandler<object>>();

const readOnlyHandler: ProxyHandler<object> = {
	/**
	 * Lazy wrap each nested property as it's own read only Proxy
	 */
	get(target, propertyKey, receiver?): any {
		const value = Reflect.get(target, propertyKey, receiver);

		if (typeof value === 'object' && value !== null) {
			const descriptor = Reflect.getOwnPropertyDescriptor(target, propertyKey);
			if (descriptor?.configurable) {
				return buildReadonlyProxy(value);
			}
		}

		return value;
	},

	/**
	 * Fail set operation when doing modification of read only object.
	 * Allow set operation if it's performed on derived object rather original one
	 */
	set(target, propertyKey, value, receiver) {
		const proxy = objectToProxyMap.get(target);
		if (receiver === proxy) {
			throw new ReadonlyObjectModification(
				`Can not set property '${String(propertyKey)}' on readonly object`,
				target
			);

			return false;
		}

		return Reflect.set(target, propertyKey, value, receiver);
	},

	/**
	 * Prevent deletion of properties from readonly object
	 */
	deleteProperty(target, propertyKey) {
		throw new ReadonlyObjectModification(
			`Can not delete property '${String(propertyKey)}' on readonly object`,
			target
		);
	},

	/**
	 * Prevent addition of new properties to read only object
	 */
	defineProperty(target, propertyKey) {
		throw new ReadonlyObjectModification(
			`Can not define property '${String(propertyKey)}' on readonly object`,
			target
		);
	},
};

/**
 * Build read only proxy which will generate nested read only proxies as needed
 * @param object
 */
export function buildReadonlyProxy<T extends object>(object: T): Immutable<T> {
	let proxy = objectToProxyMap.get(object);

	if (proxy) {
		return proxy as Immutable<T>;
	}

	proxy = new Proxy(object, readOnlyHandler);
	objectToProxyMap.set(object, proxy);

	return proxy as Immutable<T>;
}

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

No branches or pull requests

2 participants