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

SetProperty does a replace. #106

Open
dannysmc95 opened this issue Mar 22, 2023 · 12 comments
Open

SetProperty does a replace. #106

dannysmc95 opened this issue Mar 22, 2023 · 12 comments

Comments

@dannysmc95
Copy link

dannysmc95 commented Mar 22, 2023

Is there a way to make it so setProperty can merge, rather than replace?

@tommy-mitchell
Copy link

Since there are no options objects on the methods in this library, maybe a new mergeProperty could be added? I wanted to use this to test sindresorhus/write-package#21, for example.

@sindresorhus
Copy link
Owner

A .mergeProperty() method sounds good to me.

Should it do shallow or deep merge?

@tommy-mitchell
Copy link

I feel like I’d expect it to recursively merge, but maybe there can be a deep: true option?

@sindresorhus
Copy link
Owner

Should this method support arrays? If yes, it makes sense to merge the contents.

However, it does not make sense to merge the contents of the array if it's inside an object, then we should replace. So this would create an inconsistency.

@dannysmc95
Copy link
Author

dannysmc95 commented Apr 24, 2023

A .mergeProperty() method sounds good to me.

Should it do shallow or deep merge?

The issue with the current default replace strategy is that due to the way the JS Proxy/Pass by reference works (the core workings of Vue.js) when you replace a value, it removes the reference between the "reactive" value I have created and it's parent (the page object).

My suggestion would be:

mergeProperty method to recursively merge properties (I would expect deep by default personally, but up to you).
syncProperty which does the same as mergeProperty but it handles arrays slightly different by removing extra array items while still recursively merging - sounds dumb I know, but keeping that reference is what we need specifically.

Either way, we have an amended version of the library now, as we had to add extra options to some of the methods for our use cases (aside from the above) but either way, amazing library!

Happy to share our changes, or at least explain them, and maybe you can do a better job than I did, ping me on Discord if you wanna go through them Dahknee#2404.

@PopGoesTheWza
Copy link

Wouldn't addProperty be more descriptive than mergeProperty?
As per syncProperty, I like the idea but the name isn't telling, IMHO.

@sindresorhus
Copy link
Owner

Wouldn't addProperty be more descriptive than mergeProperty?

No, it doesn't make it clear that it merges an object at that path. add makes it sound like it works like set, adds a property at the given path.

@PopGoesTheWza
Copy link

it handles arrays slightly different by removing extra array items while still recursively merging

@dannysmc95 would you mind elaborating on these? Perhaps by providing sample inputs and desired output?

@dannysmc95
Copy link
Author

dannysmc95 commented Dec 28, 2023

@PopGoesTheWza Sure, so as noted in the previous response, most of it is about preserving the original object and that any references created to it, are also kept, due to the way the Proxy API works.

Small note, it's like 11pm, and I have tested none of the below, so take it with a grain of salt, it should convey what I am trying to say, but it could be off.

I am not 100% sold on the syncProperty but mergeProperty for it's use case makes a lot of sense.

MergeProperty

Imagine there is an object:

const ourObject = {
	hello: {
		world: 123,
		foo: true,
		list: [
			{
				name: 'a',
				age: 1
			},
			{
				name: 'b',
				age: 2
			}
		]
	}
}

Then we get an object to merge:

const objectToMerge = {
	worldNew: 234,
	list: [
		{
			name: 'abcdef',
			age: 3,
		}
	]
}

We would then do something along the lines of:

mergeProperty(ourObject, 'hello', objectToMerge);

Would then output something along the lines of:

const mergedObject = {
	hello: {
		world: 123,
		worldNew: 234,
		foo: true,
		list: [
			{
				name: 'abcdef',
				age: 3
			},
			{
				name: 'b',
				age: 2
			}
		]
	}
}

So when the merge occurs, it will use a recursive merge strategy meaning that there are no property reassignments other than the property it's changing.

SyncProperty

So the exact same as mergeProperty but it handles arrays differently by syncing them to the merging object.

So take this object:

{
	someArray: [
		{
			foo: 12,
		},
		{
			bar: 23,
		}
	]
}

If we then proceed to merge this:

{
	someArray: [
		{
			baz: 34,
		}
	]
}

It would result in:

{
	someArray: [
		{
			foo: 12,
			baz: 34,
		},
		{
			bar: 23,
		}
	]
}

But in this instance, I actually wanted the second array item to be removed, because as much as I am merging to maintain pass by references, I also want to "sync" to keep them the same.

So in this instance we would add a small bit of extra code to the syncProperty to remove any array items after the merge that don't match the same length as the target.

If I was to simply do a replace then all the pass by reference connections made to the objects of that array (via the Proxy API) would be lost, and reactivity would be broken.


I would note this is possibly a smaller use case then maybe what the library is intended for, but the library itself has been pretty amazing so far, we have extended it a lot for our specific application we have built including, full type hinting in the dot notation format, and then we have extended it with the above functionality alongside the ability to create missing properties in a recursive way as well.

@PopGoesTheWza
Copy link

PopGoesTheWza commented Dec 29, 2023

it's like 11pm

I'm in Paris and one hour "ahead" from you... I get the feeling.

In the MergeProperty example, when two arrays are merged:

  • iterate the array from objectToMerge and set within ourObject's array, using the same indice/index

Without your example, I would have gone with "push whatever is in objectToMerge's array into ourObject's"...

In the SyncProperty example, when two arrays are merged:

  • iterate the array from objectToMerge, not bothering about its indice/index
    1. if the value is an object, scan ourObject's array for another object with the exact same key(s)
    • Upon first match, update each key with the corresponding value from objectToMerge... and perhaps mark this object as ineligible as we keep iterating objectToMerge's array
    • if no match, shamelessly push as a new value
    1. if the value is an array? Should we try to match it with the first availlable array from ourObject's array?
    2. since functions are allowed and somewhat supported, should they be treated as an object or merged with a different strategy?
    3. scalars... how do you think they should sync?

While mergeProperty is rather straightforward and easy to document; syncProperty is still a tad fuzzy and maybe too specific. In order to start working on it, I would like @sindresorhus position on that.

@dannysmc95
Copy link
Author

dannysmc95 commented Dec 29, 2023

@PopGoesTheWza yeah, I agree that syncProperty isn't very "direct" and could possibly be a setting against the mergeProperty realistically, the only reason I separate them is because they have a drastically different behaviour when working specifically with arrays.

Funnily enough within our custom version, everything is actually an extension to the setProperty with just a settings object tacked onto the end for things like create missing, merge, sync, etc.

Functions should be treated as functions, using them via an object I think, could be, dangerous and potentially opens the door to security issues, my assumption would be that you would just use a basic reassignment for a function, not allowing prototype style replacements, at least for us we do not have a need for that - noted below, the data applied can be converted to and from JSON, therefore things like functions would never be apart of our object, but I appreciate some people may use it like this.

We also catered for Symbols, using general lookup maps, not perfect, but does a decent job.

When it comes to things like Maps, Sets, etc. We avoid this entirely, the way we are using it is purely object based, so it's actually built from JSON (via server API) and then applied into our Vue app that renders the page (dynamic server-rendered pages), so when it comes to things like that, we avoid, I am unsure how a Map/Set could work in this behaviour, I am sure there is a way, but I feel like it adds a level of complexity to something I think may not be worth coding in.

tl;dr, mergeProperty is probably better off having a setting that revolves around syncArray or something along those lines, so that the behaviour is opt-in, as by default, I wouldn't expect it, but it's something that may be needed (like our use case).

i.e.

mergeProperty(foo, 'some.path', bar, { syncArray: true });

@Richienb
Copy link
Contributor

Richienb commented Apr 23, 2024

Practically, would a mergeProperty with these options be useful?

deep: true/false
arrays: 'merge'/'append'/'replaceWhole'/'replaceEach'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants