Result of retrocycle lost when using custom reviver

[minedeljkovic]

This change ca645a9 seems to cancel the effect of retrocycle when using custom reviver, since resolved value is strigified, than parsed again.
For example if I want to let $jsan deal with Date values in retrocycle and additionally use custom reviver for some custom type, Date value will be stringifed once resolved and then parsed into string, so I end up with string instead of Date.
Is the idea of this change that you should either not use reviver at all, or to use reviver for all the values, including the ones retrocycle can process?

P.S. This line is useless after this change since needsRetrocycle variable is never used.

[alexsbryan]

I'm also curious if it's intended that when using a reviver that you won't get any of the retrocycle processing? We're using https://github.com/zalmoxisus/remotedev-serialize and it's not able to handle Date values after ca645a9

[kolodny]

Thanks for the bug report. A fix is out in v3.1.11

[zalmoxisus]

@kolodny Looks like the issue still persists in v3.1.11.

Here's an example:

jsan.parse('{"2":{"type":"PERFORM_ACTION","action":{"type":{"$jsan":"sINCREMENT"}},"timestamp":1543156067688},"3":{"type":"PERFORM_ACTION","action":{"type":{"$jsan":"sINCREMENT"}},"timestamp":1543156067839}}', (key, value)=>value)

It returns:

{ '2': { type: 'PERFORM_ACTION', action: {}, timestamp: 1543156067688 },
  '3': { type: 'PERFORM_ACTION', action: {}, timestamp: 1543156067839 } }

While without the reviver:

{ '2': 
   { type: 'PERFORM_ACTION',
     action: { type: Symbol(INCREMENT) },
     timestamp: 1543156067688 },
  '3': 
   { type: 'PERFORM_ACTION',
     action: { type: Symbol(INCREMENT) },
     timestamp: 1543156067839 } }

[artem-v-shamsutdinov]

I confirm that the issue is still open.

We are using redux-devtools, which recently reverted it's jsan depedency to 3.1.9. That broke serialization of our actions and forced us to disable redux-devtools since it was preventing the app from running (in dev mode). Serialization of the same objects works in 3.1.10 & 3.1.11

Thank you very much for the time and effort spend on looking into this issue! :)

[zalmoxisus]

I think there was a confusion here, @kolodny was looking into stringify's replacer here, while the issue introduced in #20 is about parsing's reviver. Maybe @jsmonkey could help here. I could try to solve it, but that could break the intention of that PR.

[artem-v-shamsutdinov]

Understood, thank you for looking into it! Unfortunately we do not presently have the resources needed to research the issue further and contribute the necessary fix.

If it helps, we are using:

@angular 5.2.11
@ngrx 5.2.0
ngrx-data@1.0.0-beta.13

The action that is causing the issue is 3-rd party to us. It is the "ROUTER_NAVIGATION" action from @ngrx

Thanks, :)

[zalmoxisus]

I'm suggesting to revert #20 in #26 as per the arguments provided there.

[zalmoxisus]

I published 3.1.12, which reverts #20. The purpose of that PR can be achieved by a new option refs set to false, so it won't transform references and should be handled well inside ImmutableJS custom toJSON. It can be added on remotedev-serialize side. Maybe there's a better solution for that, but we'll still need to handle non-stringified by JSON.stringify types (like Date, Infinity...) for ImmutableJS as well.

However by not recording refs, we are not handling circular references as well, so there will be an infinite recursion in that case. I suggested a fix in #27.

[zalmoxisus]

Moved @jsmonkey's tests to zalmoxisus/remotedev-serialize@7f33612 and looks like it works now as expected. It was better to have them here as it was made, but remotedev-serialize is installing previous version of jsan and is difficult to use.