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

Is immutable.js compatible with this library? #35

Closed
abhinavzspace opened this issue Nov 8, 2016 · 17 comments
Closed

Is immutable.js compatible with this library? #35

abhinavzspace opened this issue Nov 8, 2016 · 17 comments

Comments

@abhinavzspace
Copy link

Immutable.js does not seem to work with this setup. I am always getting normal objects in my selectors instead of immutable states. More chances are that I am doing something wrong, but just wanted to confirm. Please close this issue if it's compatible.

@vhmth
Copy link
Collaborator

vhmth commented Nov 8, 2016

@abhinavzspace mind posting some error traces? We should make this lib compatible with immutable

@tshaddix
Copy link
Owner

tshaddix commented Nov 8, 2016

@abhinavzspace @vhmth The way that this library works is through Chrome Message Passing. This page states:

A message can contain any valid JSON object (null, boolean, number, string, array, or object).

This is most likely your issue. At the end of the day, as your data moves between event, content-scripts, and popovers, it is being serialized with .toJSON(). This will cause Immutable objects to be turned to normal JS objects.

I think there is a way we could add configuration parameters that would allow the "immutable type" to be passed with the payload, and then have the different components parse the objects back to their expected immutable before being passed around the app.

Would essentially need configuration on ProxyStore and store wrapper that would facilitate this as data is moved between them.

@tshaddix tshaddix added this to the v1.0.0 milestone Nov 8, 2016
@tshaddix tshaddix removed this from the v1.0.0 milestone Nov 29, 2016
@EdmundLeex
Copy link
Contributor

I feel like parsing the serialized object back into immutable type on the view layer seems unnecessary. What's important is just, you can use immutable type in the store (your library seems to work with Immutable.js nicely already). Then on the react view layer, just get the state as if you are getting from a normal js object. You are not mutating it anyway. Just a thought.

@tshaddix
Copy link
Owner

tshaddix commented Jan 16, 2017

@EdmundLeex Great point! I completely agree with you :)

@abhinavzspace
Copy link
Author

@EdmundLeex I am doing exactly the way you mentioned. 👍

@abhinavzspace
Copy link
Author

Close issue button should not be too close to comment button...... :)

@abhinavzspace
Copy link
Author

BTW there is no need for compatibility with immutable.js. So it's not a real issue. Please close it if you feel like.

@tshaddix
Copy link
Owner

@abhinavzspace Sounds good! Thank you :)

@SudoPlz
Copy link

SudoPlz commented Aug 20, 2017

What about shouldComponentUpdate ? How do we know if an object is the same or wether it has changed (when it has a deep structure)?
That was another great reason for me to use immutable objects.

Thanks.

@vhmth
Copy link
Collaborator

vhmth commented Aug 20, 2017

@SudoPlz that would be orthogonal to what @EdmundLeex was mentioning. If that's the case and you are afraid of mutations at the view layer in your content and popup scripts (which is something immutable says it protects against), we should probably go with the method of allowing for immutable types in a configuration option (like @tshaddix mentioned).

I would work on this, but, since we don't use immutable.js at Loom, it's hard to justify pushing this effort forward on my end vs. working on #65 (just being real). @SudoPlz or @abhinavzspace would either of y'all be interested in extending the store to allow for immutable data types?

@SudoPlz
Copy link

SudoPlz commented Aug 21, 2017

Hmm, not sure if I got @tshaddix 's answer right, but it seems to me that whenever the immutable is passed as a message, it's being serialised, and the solution would be to manually create a new Immutable after we receive an object from the store.

But that would mean that we'd always have a new immutable object, and the nextProps.anImmutable !== this.props.anImmutable would always return true;

@EdmundLeex
Copy link
Contributor

EdmundLeex commented Aug 22, 2017

it seems to me that whenever the immutable is passed as a message, it's being serialised, and the solution would be to manually create a new Immutable after we receive an object from the store

yes, pretty much.

nextProps.anImmutable !== this.props.anImmutable shouldn't this be false if there is a difference if you just compare the serialized js object? Both would've been serialized by the time you compare them in the view layer.

@vhmth
Copy link
Collaborator

vhmth commented Aug 22, 2017

I think @SudoPlz is correct. nextProps.anImmutable would never equal this.props.anImmutable since the entire object is always serialized. This lib doesn't currently do "patches" to the store. That seems to be the only way to get this right.

@corytheboyd
Copy link

Bringing this issue back to life, as I am no longer able to use Immutable with this library. You cannot simply deserialize the store in the view layer with Immutable fromJS if your state uses non-standard data structures (i.e. OrderedMap). If there were hooks into where serialization happens on both ends of the sendMessage it wouldn't be hard to use something like https://github.com/glenjamin/transit-immutable-js to keep things Immutable across the stack.

@vhmth
Copy link
Collaborator

vhmth commented Oct 13, 2017

This got landed not too long ago:

#100

Not a fix, but should make things a bit less "horrible" for those relying on immutability (although not perfect - esp. for this case).

How are OrderedMaps usually handled in Redux? Is OrderedMap typically handled/serialized ok in other Redux apps? This is a noob question, but I'm trying to get a lay of immutable.js land.

@corytheboyd
Copy link

@vhmth Oh nice, will take a look at upgrading soon!

In your normal Redux application the Immutable OrderedMap object will retain it's shape (as in it will not be serialized before insertion into the store). This falls apart here across the chrome messaging port since it serializes complex objects. OrderedMap actually serializes to an object (which is obviously wrong, as you lose the order deserialization):

const om = new Immutable.OrderedMap([['foo', 1], ['bar', 2]])
JSON.stringify(om)
//=> "{"foo":1,"bar":2}"

@vhmth
Copy link
Collaborator

vhmth commented Nov 2, 2017

@corytheboyd this is super late, but is this still an issue for you? I won't be able to tackle this kinda functionality, but I wanna make sure this doesn't slip through the cracks.

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

6 participants