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

Support for immutable export/import #278

Closed
mlucool opened this issue Dec 28, 2016 · 50 comments
Closed

Support for immutable export/import #278

mlucool opened this issue Dec 28, 2016 · 50 comments

Comments

@mlucool
Copy link
Contributor

mlucool commented Dec 28, 2016

Although immutable.js data shows up perfectly in this tool, the export of it does not distinguish it from a plain old JSON object. This means that when I import it back, the 'preloadedState' sets the redux store incorrectly.

@zalmoxisus
Copy link
Owner

You should specify how to deserialize it. See docs and the example for deserializeState. If you have immutable data structures in the action payload, you'll also need to specify deserializeAction parameter.

@mlucool
Copy link
Contributor Author

mlucool commented Dec 29, 2016

How do I access this for the chrome store extension?

@zalmoxisus
Copy link
Owner

When you add the extension's enhancer, you can specify additional options like in that example from the docs:

const store = createStore(rootReducer, window.__REDUX_DEVTOOLS_EXTENSION__ && window.__REDUX_DEVTOOLS_EXTENSION__({
  deserializeState: (state) => ({
    todos: {
      ...state.todos,
      todoList: Immutable.fromJS(state.todos.todoList)
    }
  })
}));

@mlucool
Copy link
Contributor Author

mlucool commented Dec 29, 2016

Ah great - thanks!

@mlucool mlucool closed this as completed Dec 29, 2016
@mlucool
Copy link
Contributor Author

mlucool commented Dec 30, 2016

Hi, this is actually still an issue with the export/import.

Here is a snip-it to try on the todomvc example that change every number to foo or bar:

diff --git a/examples/todomvc/store/configureStore.js b/examples/todomvc/store/configureStore.js
index b483cf4..e943509 100644
--- a/examples/todomvc/store/configureStore.js
+++ b/examples/todomvc/store/configureStore.js
@@ -4,7 +4,11 @@ import * as actionCreators from '../actions';
 
 export default function configureStore(preloadedState) {
   const enhancer = window.__REDUX_DEVTOOLS_EXTENSION__ &&
-    window.__REDUX_DEVTOOLS_EXTENSION__({ actionCreators });
+    window.__REDUX_DEVTOOLS_EXTENSION__({ actionCreators,
+        serializeState: (key, value) => typeof value === 'number' ? 'foo' : value,
+        serializeAction: (key, value) => typeof value === 'number' ? 'bar' : value
+});
+

In the UI this does work as expected, but when I download it looks like:

{
	"payload": "[{\"type\":\"DELETE_TODO\",\"id\":0}]",
	"preloadedState": "{\"todos\":[{\"id\":0,\"completed\":false,\"text\":\"sdf\"}]}"
}}

Instead of:

{
	"payload": "[{\"type\":\"DELETE_TODO\",\"id\":\"bar\"}]",
	"preloadedState": "{\"todos\":[{\"id\":\"foo\",\"completed\":false,\"text\":\"sdf\"}]}"
}}

Thanks for looking into this.

@mlucool mlucool reopened this Dec 30, 2016
@zalmoxisus
Copy link
Owner

You should use deserializeState / deserializeAction for your case, as you want to change the data that comes back to the client side.

serializeState is used to change how the data appears on the monitor. However, when exporting to a file, serializeState is always true.

@mlucool
Copy link
Contributor Author

mlucool commented Dec 30, 2016

Why is serializeState is always true for a file? Can't it use the same rules as display? I ask this because my goal is to write a generic immutable->file format like:

function seralize(key, value) {
    if (value && value.toJS) {
        const ret = value.toJS();
        ret.__isImmutable = true;
        return ret;
    }
    return value;
}

Then when I deseralize the state, I can convert it back into immutable dynamically too.

@zalmoxisus
Copy link
Owner

zalmoxisus commented Dec 30, 2016

We force it to true in order to guarantee that we'll not loose unserializable data like circular references and functions, also we handle ImmutableJS in this case as well.

But I see what you want to accomplish. Will include the custom function there if specified.

@mlucool
Copy link
Contributor Author

mlucool commented Dec 30, 2016

Thanks!

zalmoxisus added a commit that referenced this issue Jan 3, 2017
@zalmoxisus
Copy link
Owner

Implemented in v2.12.0. Feel free to reopen the issue if it doesn't work as expected.

A PR to update docs on how to better work with Immutable would be much appreciated.

@mlucool
Copy link
Contributor Author

mlucool commented Jan 4, 2017

Just tested and it does work. I was not expecting it all to be jsan encoded still. Not sure if that is the right thing to do or not with a custom seralizalizer. It does means I'll need to write some translators to be able to use tools like jq to search it

@mlucool
Copy link
Contributor Author

mlucool commented Jan 4, 2017

It also does not seem to honor the deseralizer during import. The export matches expectations (although jsan).

I have found a few variants of it failing and I think it has to do with the deseralize not being called. I can also get something related to 'Unexpected key "__isImmutable" found in previous state received by the reducer', but am unable to reproduce this in a small example.

Here is a starter example that I think should work. I am testing via just the todomvc example. I make a change, commit it. Make a change then download.

--- a/examples/todomvc/store/configureStore.js
+++ b/examples/todomvc/store/configureStore.js
@@ -1,10 +1,17 @@
 import { createStore } from 'redux';
 import rootReducer from '../reducers';
 import * as actionCreators from '../actions';
+import _ from 'lodash';
 
 export default function configureStore(preloadedState) {
   const enhancer = window.__REDUX_DEVTOOLS_EXTENSION__ &&
-    window.__REDUX_DEVTOOLS_EXTENSION__({ actionCreators });
+    window.__REDUX_DEVTOOLS_EXTENSION__({ actionCreators,
+        serializeState: serialize,
+        serializeAction: serialize,
+        deserializeState: deserialize,
+        deserializeAction: deserialize
+});
+
   if (!enhancer) {
     console.warn('Install Redux DevTools Extension to inspect the app state: ' +
       'https://github.com/zalmoxisus/redux-devtools-extension#installation')
@@ -22,3 +29,40 @@ export default function configureStore(preloadedState) {
 
   return store;
 }
+
+
+function serialize(key, value) {
+  if(_.isObject(value)) {
+     return {...value, __somePrivate: true};
+  }
+  return value;
+}
+
+function deserialize(obj) {
+    console.log('Deserializing %o', obj);
+    if (_.isObject(obj)) {
+        return _.mapValues(obj, value => {
+            if (value && value.__somePrivate) {
+                return _.omit(value, '__somePrivate');
+            }
+            return value;
+        });
+    }
+    return obj;
+}
+
+/*** Test ****/
+const o = {
+    a: {b: {c: 'd'}}
+}
+
+console.log('Original');
+console.log(o);
+console.log('Serialized');
+console.log(doSerialize(o));
+console.log('Inversed');
+console.log(deserialize(doSerialize(o)));
+
+function doSerialize(obj) {
+    return _.mapValues(obj, (value, key) => serialize(key, value));
+}

I am also slightly confused about the lack of symmetry between serialize and deseralize so I might be doing something wrong. It would be good if I could provide both in the (key, value) format OR the (obj) format.

@zalmoxisus zalmoxisus reopened this Jan 5, 2017
@zalmoxisus
Copy link
Owner

zalmoxisus commented Jan 5, 2017

@mlucool, thanks for investigating it.

Yes, I've broken it in dcd346e. I'll look into it.

Regarding of the lack of the symmetry, the serialize function is passed as the replacer to JSON.stringify thus it's signature. The deserialize parameter was introduced earlier, though we could use the same signature and even pass it to JSON.parse as the reviver. However, it would be a breaking change without back compatibility. Initially, they were not meant to be used together. serialize* is for sending the data to the monitor, deserialize* is how the imported / persisted data.

zalmoxisus added a commit that referenced this issue Jan 5, 2017
Related to #278 and
dcd346e79e
6fb1571f53d64f2d6d462128c205d7
zalmoxisus added a commit that referenced this issue Jan 5, 2017
@mlucool
Copy link
Contributor Author

mlucool commented Jan 5, 2017

Thanks for reopening.

Although not ideal, you could detect the number of arguments (or use safer yet use some flag) to make it symmetric and backwards compatible. You could also consider making a breaking change and being declarative about it. I'd prefer the way it is done for deserialize as you feel like you get more control by getting in a whole object and then doing whatever you want to return a serialized version.

@zalmoxisus
Copy link
Owner

I've fixed the issue with no calling deseralizer during the import in a5e82ab.

Before publishing a patch, I'd like to clarify the following:

I was not expecting it all to be jsan encoded still. Not sure if that is the right thing to do or not with a custom seralizalizer.

JSAN takes care of unserializable data (otherwise JSON.stringify will just throw). Immutable data contains functions, which would be "encoded" as well, but since you transform them through your custom serialize* method they will not altered by jsan.

I can also get something related to 'Unexpected key "__isImmutable" found in previous state received by the reducer', but am unable to reproduce this in a small example.

That's because it was added to the root reducer, thus the error from Redux. If the the root reducer is meant to be immutable, then that was caused by the fact that deserializer wasn't called.

I am also slightly confused about the lack of symmetry between serialize and deseralize so I might be doing something wrong. It would be good if I could provide both in the (key, value) format OR the (obj) format.

As per the previous message, we could add an additional parameter for deseralize function, to pass it as a reviver, and to have the same (key, value) format. The current format is handy as it makes easier to transform the necessary reducers from the object, or transform the root reducer, and also deserializeState / deserializeAction are compatible with Redux DevTools persist enhancer which consumes them. Do you think it's worth extending the API with another parameter?

@zalmoxisus
Copy link
Owner

zalmoxisus commented Jan 5, 2017

I'd prefer the way it is done for deserialize as you feel like you get more control by getting in a whole object and then doing whatever you want to return a serialized version.

The reason for (key, value) format in JSON.sringify replacer, is that data can invoke a custom toJSON method before passing the result to the replacer, so the value from that function can be different from the value from original object. Here are some examples.

@zalmoxisus
Copy link
Owner

Although not ideal, you could detect the number of arguments (or use safer yet use some flag) to make it symmetric and backwards compatible.

If I'm not missing anything, detecting the number of arguments isn't trivial, we have to stringify the function.

We could just pass to serializeState / serializeAction an object with configuration like { replacer: function, transform: function, options: object }.

@mlucool
Copy link
Contributor Author

mlucool commented Jan 6, 2017

For length you should be able to do this.

I'll be very happy with whatever way you think is better as long as it is symmetric; I see reasons for both. The configuration object sounds ok too!

As for JSAN, I understand why it is a great default. The problem is its JSAN inside of JSAN (please correct me if I am wrong). So I need to parse the JSAN (which requires a JS parser) then parse each field back too. This work is very straightforward, but in my small universe we only use plain old data and having things in pure JSON structure means other tools work out of the box. To be safe, we could use JSON with something like this or this. It would be better if we could at least have the option to opt out, if you don't think it will complicate things too much.

@zalmoxisus
Copy link
Owner

zalmoxisus commented Jan 6, 2017

@mlucool, thanks for the details. Then we could use deserializeState / deserializeAction as both transformer and reviver. Also I'd add another parameter serialize, which would contain replacer and reviver, and would be applied for both states and actions. So instead of passing functions we could use one object. So we could use it like so:

const store = createStore(rootReducer, window.__REDUX_DEVTOOLS_EXTENSION__ && window.__REDUX_DEVTOOLS_EXTENSION__({
  serialize: serializeImmutable
}));

I'll come with an implementation of serializeImmutable, as just toJS / fromJS wouldn't handle all the cases.

As for JSAN, the object is passed to deserializeState / deserializeAction after jsan.parse, so it shouldn't be an issue. It could be a problem for the reviver we want to introduce, but assuming that our custom replacer already took care of our data it should be ok.

@mlucool
Copy link
Contributor Author

mlucool commented Jan 6, 2017

Nice search! I guess my point is this replacer/reviver have nothing to do with remotedev and I think there is are great usecases outside of remote devtools (great work here btw!).

zalmoxisus added a commit that referenced this issue Jan 9, 2017
@zalmoxisus
Copy link
Owner

zalmoxisus commented Jan 9, 2017

Well, it took more than I expected, but the result is amazing.

In addition to importing and persisting immutable data, we'll be able to investigate the datatypes on the monitor:
screen shot 2017-01-08 at 1 00 22 pm

Here's the data and how it gets stringified.

The implementation is pretty simple. I guess we could afford 50 lines of code to include into the extension to make the life easier. So will allow just to pass the Immutable library like so:

import Immutable from 'immutable';
const store = createStore(rootReducer, window.__REDUX_DEVTOOLS_EXTENSION__ && window.__REDUX_DEVTOOLS_EXTENSION__({
  serialize: {
    immutable: Immutable
  }
}));

The lib can be used apart from the extension to stringify / parse the immutable data (see the tests). As per __remotedevType__, we could pass an arbitrary prefix, if someone needs it and will send a pr.

The only data type, which is not implemented yet, is Record. We don't have any information about the used class when persisting and don't know how to create it. I figure a solution would be to pass keys to refer a specific class like so:

import Immutable from 'immutable';
const store = createStore(rootReducer, window.__REDUX_DEVTOOLS_EXTENSION__ && window.__REDUX_DEVTOOLS_EXTENSION__({
  serialize: {
    immutable: Immutable,
    refs: { myRecord: ABRecord }
  }
}));

So, we'll be able to parse back obj:

var ABRecord = Immutable.Record({a:1, b:2, c:3})
var obj = { myRecord: ABRecord({a:1, b:2}) }

@mlucool
Copy link
Contributor Author

mlucool commented Jan 9, 2017

This seems great. I don't see where I would inject serialize: {immutable: Immutable} in this commit - was that just an example?

As for __remotedevType__ what if we called it __serializedType__ or something similar? Feels better for a library called serialize-immutable

I personally never use Records so I don't have a lot of insight there.

Thanks again for all the work on this 👍

@zalmoxisus
Copy link
Owner

Yes, that's not there yet. Still was thinking whether we should import it as a library and add to serialize parameter or include it in the injected script.

OK, will use __serializedType__/__serializedRef__ (though I still think we should keep an unique prefix everywhere to avoid collisions).

Now searching if Record is widely used, I found transit-immutable-js, which does this job already and pretty well. Seems like we're not introducing nothing new here, except that nice preview of types in the Inspector monitor.

Since we can reuse this functionality for ES6 Map/Set and TypedArrays, and maybe other custom types (for example, like dealing with Record class we could persist a React class from action's payload), will name it remotedev-serialize and include inside the extension, Remote Redux DevTools and Remotedev.

zalmoxisus added a commit that referenced this issue Jan 10, 2017
zalmoxisus added a commit that referenced this issue Jan 10, 2017
@zalmoxisus
Copy link
Owner

zalmoxisus commented Jan 10, 2017

I have published 2.12.1 to Chrome Store, which implements this.

You can use it like so:

import Immutable from 'immutable';
const store = createStore(rootReducer, window.__REDUX_DEVTOOLS_EXTENSION__ && window.__REDUX_DEVTOOLS_EXTENSION__({
  serialize: {
    immutable: Immutable,
    // refs: [ABRecord] - for Records
  }
}));

It also allows to persist the state.

Let me know if you have any issues with that.

@mlucool
Copy link
Contributor Author

mlucool commented Jan 10, 2017

The formatting looks great, but I do run into issues with using the extension now. I love the fact that I can now just paste output into a JSON formatter and explore because of the lack of JSAN.

I have noticed the following issues:

  1. It doesn't always show data anymore and I need to refresh a few times to get it to work. I get a screen as if nothing redux has ever happened - including no @init. I have yet to restart my computer though.
  2. When I do see data, it's the attached log monitor, but never in the Remote DevTools

If you have not seen these, I can try and do a reboot sometime in the next 24 hours.

@zalmoxisus
Copy link
Owner

zalmoxisus commented Jan 10, 2017

I'm confused about Remote DevTools. Are you using the extension's enhancer or remote-redux-devtools? Does the problem occur only when using serialize parameter?

@zalmoxisus
Copy link
Owner

I tried also the version from Chrome Store and cannot see that happening.

If you have that problem with the extension's enhancer, check Developer mode on chrome://extensions/ and click on background page bellow the extension's name. There should be an exception.

If it's about remote-redux-devtools and you're not using a local server, maybe it's just our server being unavailable, better to use it with remotedev-server.

@mlucool
Copy link
Contributor Author

mlucool commented Jan 10, 2017

I was referring to: 'Open in Remote DevTools (Alt+Shift+Up Arrow)'

Thanks for the tip on where to find the error. I am seeing this there:

_generated_background_page.html:1 Error in event handler for (unknown): TypeError: Cannot read property '0' of undefined
    at a (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:1:10037)
    at f (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:1:12563)
    at chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:1:7357
    at f (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:1:3185)
    at Object.dispatch (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:3:20324)
    at f (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:3:19071)
    at n (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:3:19568)

@zalmoxisus
Copy link
Owner

That error says that computedStates from the payload you're sending is undefined or cannot be parsed. Please provide a repro or at least a widget of how you configured the store.

@zalmoxisus
Copy link
Owner

zalmoxisus commented Jan 10, 2017

Seems like our introduced reviver failed for some reason, so you got undefined.

As you can see from the code above, it could happen when value.data is null, so we cannot add __serializedType__ parameter to it. Not sure what we should do in this case, and don't know how an Immutable data can become null after serialization.

If you cannot provide a repro or at least an example state to reproduce the issue, maybe you could add a test to reproduce that here?

@mlucool
Copy link
Contributor Author

mlucool commented Jan 10, 2017

I'll try and reproduce that test or provide something you can work from

@mlucool
Copy link
Contributor Author

mlucool commented Jan 10, 2017

I am struggling to reproduce this in any meaningful way outside my app. Where would I expect to see this line. The normal console? I want to see what data it rejects.

@zalmoxisus
Copy link
Owner

@mlucool, that script is running in the extension's background page. You could listen for events (messages) sending from the extension, by adding a listener to you page:

window.addEventListener('message', (event) => { if (event.data) console.log(event.data) } , false);

There will be some keys of the immutable objects, which you can try to parse with our reviver. I suspect there should be somewhere { data: null, __serializedType__: '...' }, which we should investigate.

@mlucool
Copy link
Contributor Author

mlucool commented Jan 11, 2017

Humm so I have run this and the most interesting/unexpected finding thus far is something like in the computed state string:

"appMetaData":{  
    "$jsan":"$[0].state.app.data.appMetaData"
},

Also this:

"example":{  
    "data":{  
        "datetime":{  
            "$jsan":"d1484148409159"
        }
     },
     "__serializedType__":"ImmutableMap"
}

appMetaData I expect to be an empty map (possibly null?).

@zalmoxisus
Copy link
Owner

In the first example it's just a reference to state.app.data.appMetaData, it could be anything you have there, so no information here.

In the second example, it's how jsan encodes the Date format (which is not serializable by JSON.stringify). I guess you have { example: Immutable.Map({ datetime: new Date()}) }, which works as expected. The problem is not here.

I'd suggest to exclude all reducers one by one and see which one fails. Then put there some random data you can share, so I could investigate.

@mlucool
Copy link
Contributor Author

mlucool commented Jan 11, 2017

Thanks for the tip. I have gotten it down to a bug in my code where if I change one thing I can fix/reproduce the bug. It is related to appMetaData (maybe by chance).

appMetaData had a bug where it's init state was a plain JS object (but future states which I don't think it ever got to are Immutable). It sat within a higher order reducer whose data was immutable. When I changed the init state to Map() this plugin does work and the export looks like:

{
  "payload":"[]",
  "preloadedState":"{\"app\":{\"data\":{\"appMetaData\":{\"data\":{},\"__serializedType__\":\"ImmutableMap\"}},\"__serializedType__\":\"ImmutableMap\"},\"routing\":{\"locationBeforeTransitions\":{\"pathname\":\"/\",\"search\":\"\",\"hash\":\"\",\"state\":null,\"action\":\"POP\",\"key\":\"8xg0op\",\"query\":{},\"$searchBase\":{\"search\":\"\",\"searchBase\":\"\"}}}}"
}

This seems to imply that there is an issue when a regular object is within an immutable map. I have tried to create a test for this, but my simple test of parsing a Map with an object in it succeeded when I thought it would fail.
I.e. These both work:

console.log(parse(`{\"app\":{\"data\":{\"appMetaData\":{\"data\":{},\"__serializedType__\":\"ImmutableMap\"}},\"__serializedType__\":\"ImmutableMap\"}}`));
console.log(parse(`{\"app\":{\"data\":{\"appMetaData\":{\"data\":{}}},\"__serializedType__\":\"ImmutableMap\"}}`));

Please let me know if this was not clear or if there is anything else you suggest I try to help you be able to reproduce it.

@zalmoxisus
Copy link
Owner

Thanks for the details and the investigation! Having plain objects inside immutable maps shouldn't be an issue. It would be much appreciated if could find some time to share an example (a repo or jsfiddle), I can run and reproduce that issue.

@mlucool
Copy link
Contributor Author

mlucool commented Jan 11, 2017

Yeah I can't quite figure out why yet. I am still working to make something that I can share, was updating with the hope it would be obvious to you

@mlucool
Copy link
Contributor Author

mlucool commented Jan 11, 2017

Got it:

diff --git a/examples/todomvc/reducers/index.js b/examples/todomvc/reducers/index.js
index f4aad18..10cd4c2 100644
--- a/examples/todomvc/reducers/index.js
+++ b/examples/todomvc/reducers/index.js
@@ -1,8 +1,27 @@
 import { combineReducers } from 'redux';
 import todos from './todos';
+import {Map} from 'immutable';
+
+function combineImmutableReducers(reducers) {
+    const combinedReducers = combineReducers(reducers);
+
+    return (state, action) => Map(combinedReducers(
+        Map.isMap(state) ? state.toObject() : state, action
+    ));
+}
+
+
+function yReducer(state = {}, action) {
+  return state;
+}
+
+function xReducer(state = Map(), action) {
+  return state;
+}
 
 const rootReducer = combineReducers({
-  todos
+  todos,
+  bar: combineImmutableReducers({x: xReducer, y: yReducer})
 });
 
 export default rootReducer;
diff --git a/examples/todomvc/store/configureStore.js b/examples/todomvc/store/configureStore.js
index b483cf4..ce11594 100644
--- a/examples/todomvc/store/configureStore.js
+++ b/examples/todomvc/store/configureStore.js
@@ -1,10 +1,17 @@
 import { createStore } from 'redux';
 import rootReducer from '../reducers';
 import * as actionCreators from '../actions';
+import _ from 'lodash';
+import Immutable from 'immutable';
 
 export default function configureStore(preloadedState) {
   const enhancer = window.__REDUX_DEVTOOLS_EXTENSION__ &&
-    window.__REDUX_DEVTOOLS_EXTENSION__({ actionCreators });
+    window.__REDUX_DEVTOOLS_EXTENSION__({ actionCreators,
+     serialize: {
+                    immutable: Immutable
+                }
+});
+
   if (!enhancer) {
     console.warn('Install Redux DevTools Extension to inspect the app state: ' +
       'https://github.com/zalmoxisus/redux-devtools-extension#installation')
@@ -22,3 +29,4 @@ export default function configureStore(preloadedState) {

At first it works, but add one item to the list and you will see it does add in the UI (i.e. redux works), but the extension stops showing new updates.

Errors:

_generated_background_page.html:1 Error in event handler for (unknown): TypeError: Cannot read property 'length' of undefined
    at a (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:1:10447)
    at f (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:1:12563)
    at chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:1:7357
    at f (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:1:3185)
    at Object.dispatch (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:3:20324)
    at f (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:3:19071)
    at n (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:3:19568)

_generated_background_page.html:1 Error in event handler for (unknown): TypeError: Cannot read property '0' of undefined
    at a (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:1:10037)
    at f (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:1:12563)
    at chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:1:7357
    at f (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:1:3185)
    at Object.dispatch (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:3:20324)
    at f (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:3:19071)
    at n (chrome-extension://lmhkpmbekcpmknklioeibfkpmmfibljd/js/background.bundle.js:3:19568)

@zalmoxisus
Copy link
Owner

@mlucool, thanks! You were right, the problem is when having references like

"appMetaData":{  
    "$jsan":"$[0].state.app.data.appMetaData"
},

Our reviver moves appMetaData from data and the path is altered, so jsan fails to restore the reference.

A solution would not to use the reviver here, and traverse the object after it got parsed. But it would be less performant and we'll have an infinite recursion in case of circular references. I'll think into it.

@mlucool
Copy link
Contributor Author

mlucool commented Jan 13, 2017

Any further thoughts?

@zalmoxisus
Copy link
Owner

@mlucool, do you still experience that issue? I've posted 2.12.2.1, which should include the fix from kolodny/jsan#10.

@mlucool
Copy link
Contributor Author

mlucool commented Jan 14, 2017

Just played around with it some more. As far as I can tell this looks great! I'll continue to use it over the coming weeks and let you know if I run into any more issues. Thanks again for all the work here!

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

2 participants