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

Refactor reactive dict into a serializing and non-serializing version #4120

Closed
wants to merge 3 commits into from

Conversation

stubailo
Copy link
Contributor

@stubailo stubailo commented Apr 4, 2015

  • Until now, ReactiveDict always serialized and deserialized everything when getting and setting. This means you can't store any values that are not serializable, for example functions and objects with prototypes
  • With this PR, the new default is to not serialize
  • If you pass a string to the constructor, ReactiveDict will give you back a SerializingReactiveDict to preserve backwards compatibility with migrating ReactiveDict (like Session)
  • You can also call the SerializingReactiveDict constructor directly
  • ReactiveDict#equals doesn't work on a non-serializing reactive dict, because it is impossible to implement it efficiently

Breaking changes

  • If you construct a ReactiveDict with no argument to the constructor, you will not be able to use equals

Possible solution

  • Keep ReactiveDict as it is, and introduce a new class called NonSerializingReactiveDict, but that seems a little silly

Sashko Stubailo and others added 2 commits April 3, 2015 16:29
SerializingReactiveDict is the one we used to have
ReactiveDict is now non-serializing by default

Conflicts:
	packages/reactive-dict/package.js
	packages/reactive-dict/reactive-dict-tests.js
	packages/reactive-dict/reactive-dict.js
@dgreensp
Copy link
Contributor

dgreensp commented Apr 4, 2015

equals is only specced to work on primitives, so what's the problem?

Incidentally, it's now possible to make SerializingReactiveDict#equals work efficiently on all EJSON types, because EJSON.stringify has a canonical option that it didn't have when this code was first written! (See the comment about how we can only allow primitives because of key order in serialized objects.) We talked about doing that eventually, but one reason to keep equals as is (only working on primitives) would be so that it also works on a non-EJSON ReactiveDict like you are making. It seems like we should do one or the other, though, not have equals work only on primitives and ban it on non-serializable ReactiveDicts.

I think it's confusing for new ReactiveDict to return different classes with different capabilities depending on the number of arguments, unless the string-argument form is deprecated.

How about MigratedReactiveDict instead of SerializingReactiveDict? We could also work the word "session" in there somehow, evoking Session and sessionStorage. ReactiveSession or ReactiveSessionDict. But I think I like MigratedReactiveDict better.

@stubailo
Copy link
Contributor Author

Going to wait until more product direction on Tracker/reactivity.

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.

3 participants