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

add a built-in copy helper #2

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

add a built-in copy helper #2

wants to merge 1 commit into from

Conversation

Rich-Harris
Copy link
Collaborator

This adds a built-in copy function that can be used to clone the data a lens is being applied to using structural sharing. It avoids the performance overhead of the JSON hack, while bypassing the limitations around cyclical structures and JSON-supported data types, and is simpler and more performant than solutions like _.cloneDeep (which need to keep track in memory of which values have already been seen, to avoid a stack overflow).

Since subtrees are shared between the before and after, this is also kinder on the heap, if you're hanging on to old values for (e.g.) an undo stack.

Question: should it handle other types like Set and Map? Or should that be solved in userland?

@vijithassar
Copy link
Owner

There should probably be a built-in copy helper which ideally would be enabled by default, but I am not sure whether this implementation — or any implementation, for that matter — is intrinsically better than all the others.

@vijithassar
Copy link
Owner

probably gonna be a yes to Map eventually, but I am not really sure what Set would look like since there aren't indices

@Rich-Harris
Copy link
Collaborator Author

It's intrinsically better than JSON.parse(JSON.stringify(data)), which fails with cyclical structures and many data types, and it's intrinsically better than deep cloning approaches.

Apart from Map (which would be easy enough to add), where does it fall short? If the answer is 'it fails with instances of class Foo{}' (which no built-in solution could possibly accommodate) I'd argue that's out of scope for microlens anyway.

@vijithassar
Copy link
Owner

vijithassar commented Nov 28, 2018

I don't disagree with you regarding performance, but simply by virtue of being a custom algorithm in the first place, this behavior falls short of being totally obvious and intuitive, and arguably that should be a requirement of any operation that would be both hidden from the caller per the API and then also automatically applied to all inputs. It is quite possible that no solution to this problem even could qualify as totally obvious and intuitive, given the peculiarities of the language!

Would you say the implementation in this pull request is intrinsically better than structured clone?

@vijithassar vijithassar mentioned this pull request Nov 28, 2018
@Rich-Harris
Copy link
Collaborator Author

Not strictly better, insofar as structured clone handles more data types. But in terms of performance and memory characteristics, yes

@Rich-Harris
Copy link
Collaborator Author

(and we can add the missing data types so that it is strictly better)

@vijithassar
Copy link
Owner

The structured clone algorithm is officially part of the HTML5 specification. Does that standardization make it a better candidate for this kind of hidden use case?

@Rich-Harris
Copy link
Collaborator Author

I don't think so, because it's not designed for this — it's designed for serialization. Besides, the browser's implementation isn't exposed, except via hacks involving writing data to IndexedDB and then reading it back out again, or transferring it to a worker. (Both of which are asynchronous and therefore unsuitable for this purpose.)

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.

None yet

2 participants