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

Preserve object prototypes #71

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

magneticflux-
Copy link

Closes #69
Supersedes #70

@blacktaxi
Copy link
Member

Hi! Sorry for taking so long to respond, and thanks for looking into this.

Originally we didn't expect to use anything but plain JSON objects in atoms, mostly due to difficulties around consistently defining equality, update semantics and type safety. I don't immediately see why it wouldn't work with a case like you presented in the unit test (thanks for the tests BTW!), but I feel there might be issues with such implementation in general. For example if there are side effects in the class constructor, they won't be executed when you Object.create a copy. This might be totally fine (especially if you don't expect to have side effects in the constructor) in a particular use case, but I'd consider such behavior inconsistent in general.

Could you elaborate a bit more on what's your actual use case? What does you app's state look like?

Thanks!

@magneticflux-
Copy link
Author

My app's state is similar to the test cases: a data class with some extra pure functions attached. However, my app does some polymorphic array serialization and deserialization, so type information needs to remain attached for https://github.com/typestack/class-transformer to work properly.

I agree Object.create isn't ideal for modifying an immutable object, but I think it works at least as well as the for-loop that existed prior. Updating immutable data isn't easy, and a fully "correct" solution would likely involve generating specialized lens implementations for each type that needs special serialization/deserialization. That definitely isn't in scope, but I think this is an easy way to get a little bit closer to that goal.

@blacktaxi
Copy link
Member

Makes sense. In principle I'm fine with this change if:

  1. It helps with your use case. (I wouldn't endorse such usage though.)
  2. Doesn't introduce a performance regression. Would you mind setting up a jsperf page to confirm this?

@magneticflux-
Copy link
Author

I tried to set up a jsperf page, but I ran into this issue: jsperf/jsperf.com#527
I used a different service for now: https://www.measurethat.net/Benchmarks/Show/8584/0/objectassign-vs-iteration-performance-grammarly-focal

The performance difference seems negligible, since the results only range from ~0.4μs to ~1.2μs. If you want, I could change it to the code for "Object.assign with blank object and prototype prop", which is slightly faster than my original code and does the same thing (I think).

I've already tested a local version and it correctly preserves object prototypes.

@blacktaxi
Copy link
Member

Thanks! I've ran the benchmark a couple times in different browsers, here are the results:

Chrome 83:
image

Safari 13.1
image

Firefox 72
image

It seems that preserving the prototype in this way makes it almost two times slower for this particular case. I've tried adding more properties to the updated object, and now iteration seems to be the slowest:

image

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.

Classes in Atoms lose their functions after modification
2 participants