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

debug_session with immutable.js #53

Closed
zivni opened this issue Feb 8, 2016 · 6 comments
Closed

debug_session with immutable.js #53

zivni opened this issue Feb 8, 2016 · 6 comments

Comments

@zivni
Copy link

zivni commented Feb 8, 2016

It seems that after the immutable.js fix 35 every thing get serialized to plain JS. but now when using ?debug_session=ds, it doesn't deserialized back to immutable.js and i'm getting .get is not a function.

Is there a way to tell the extension to deserialize back to immutable.js, and still keep the state in the panes plain JS?

@zalmoxisus
Copy link
Owner

The extension shouldn't send back anything, it just gets changes from the lifted store, and dispatch actions to the lifted store in order to change anything. Default serialization introduced in #35, can be disabled in settings, but that problem you described comes not from the extension's serializations, but from persistState's.

The solution would be to remove persistState from the extension, so you'll be able to import it from redux-devtools and pass your deserializeState function, but I'm not sure that will help, will it?

@ganmor
Copy link
Contributor

ganmor commented Feb 21, 2016

I got it working by passing down the following deserialisation function to persistState

[Edited]

(data) => {

        let state = {};

        if (data) {
          Object.keys(data).forEach(
            dataKey => {
            state[dataKey] = Immutable.fromJS(data[dataKey]);
          });
        }
        return state;
      }

for now I am running the extension locally but maybe we could do something like this:

compose(
    instrument(subscriber),
    persistState(
      getPersistSession(),
      window.devToolDeserialiseFunc
    )
  )(next);

where devToolDeserialiseFunc have to be in the window otherwise persist state would just use identity like it does now

persistState(sessionId, deserializeState = identity, deserializeAction = identity) {

@zalmoxisus
Copy link
Owner

I think we could pass the deserialisation function to the extension like so:

window.devToolsExtension({ devToolDeserialiseFunc })

This function can be used not only for the persistState but also when importing state history from a file (after implementing #51).

Another option would be to have this function inside the extension (avoiding boilerplate) and pass a flag which will require it as

window.devToolsExtension({ isImmutableJS })

Or maybe we can even identify if the state contains ImmutableJS directly?

@ganmor
Copy link
Contributor

ganmor commented Feb 25, 2016

If you want to package the function with the extension you'd have to package immutable too, I am not sure it's a good option.
I think the first solution is better.

I'd be happy to submit a PR including:

  • The proposed change to window.devToolsExtension
  • An example of the function in the README

Also it would be great to output a warning refering to the readme if we try to serialize immutable data.

For the recored redux persist solves this be triggering an event when deserializing data. This event can be caught by the reducers to regenerate data
see
But that would be some code in the reducers just to handle debugging.

@zalmoxisus
Copy link
Owner

Agree, we don't want to package and inject Immutable.js in every page. Growing the injected script size would lead to a latency in loading and the store will be created before the window.devToolsExtension is injected.

Using the extension as

window.devToolsExtension({ deserialize:  state =>  Immutable.fromJS(state) })

would be pretty neat. The PR would be welcome. Also the parameter should be specified in the API section (we have name parameter there).

@zalmoxisus
Copy link
Owner

Implemented in #65. Should work in 1.0.0.8, thanks to @minedeljkovic.

Please see the API section and give it a try.

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

No branches or pull requests

3 participants