packages icon indicating copy to clipboard operation
packages copied to clipboard

[webview_flutter_wkwebview] Fixes JSON.stringify() cannot serialize cyclic structures

Open LinXunFeng opened this issue 3 months ago • 9 comments

Using the replacer parameter of JSON.stringify() to remove cyclic object to resolve the following error.

TypeError: JSON.stringify cannot serialize cyclic structures.

~~Related solution: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cyclic_object_value~~

Related issue: https://github.com/flutter/flutter/issues/144535

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

LinXunFeng avatar Mar 06 '24 04:03 LinXunFeng

This is in wkwebview but maybe @ditman could review the js code?

jmagman avatar Mar 06 '24 21:03 jmagman

Can do, thanks for the mention @jmagman, I normally disregard webview stuff because I end up flagged as an owner (but I'm not :P)

Ahem:

  • https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Cyclic_object_value#circular_references

ditman avatar Mar 08 '24 00:03 ditman

The biggest problem I see with this is that this is a "lossy" fix; what you deserialize is not what you were serializing; "[Cyclic]" might be good for the json encoding to not crash, but it doesn't tell you how to recover that cycle when you parse JSON back to an object?

(Also if the cycle is not important: maybe remove it before attempting to serialize to JSON?)

((PS: I don't think I've had to deal with JSON malformed like this before, but maybe with something like GraphQL you could end up with a circular dependency on your data, not sure...))

ditman avatar Mar 08 '24 19:03 ditman

The biggest problem I see with this is that this is a "lossy" fix; what you deserialize is not what you were serializing; "[Cyclic]" might be good for the json encoding to not crash, but it doesn't tell you how to recover that cycle when you parse JSON back to an object?

(Also if the cycle is not important: maybe remove it before attempting to serialize to JSON?)

Yea, I was considering this as well. But do think it is reasonable for a user to expect that console.log should be able to handle any object? Does the problem lie with us using JSON.stringify() instead of just using String()? I would consider calling console.log as the equivalent of calling print(...) in a Flutter app and would be surprised if this caused an error on recursion. It might be a breaking change to move away from JSON.stringify() now though, so my reasoning was to avoid the crash as the best solution right now.

bparrishMines avatar Mar 08 '24 20:03 bparrishMines

Looking at the issue and the Android output, it looks like Android basically just calls String() on an object passed to the console. Android just returns [object Object] when passed a recursive object.

bparrishMines avatar Mar 08 '24 20:03 bparrishMines

Ah! I didn't realize this was to implement console.log. In that case, this is probably fine.

The way this works in the browser is by it not attempting to "toString" the whole thing, it just gives you a tree-like structure for the object:

Screenshot 2024-03-08 at 12 11 24 PM

(You get tired of expanding self before the browser runs out of memory, I think :P)

The PR is a C+P from MDN however; @stuartmorgan licensing concerns?

ditman avatar Mar 08 '24 20:03 ditman

The PR is a C+P from MDN however; @stuartmorgan licensing concerns?

What is done in this PR is not permitted in Flutter.

This code is not complicated enough to warrant us adding a third_party directory to this plugin; we should get a clean implementation of the webkit_webview_controller.dart change (by giving someone who has not seen this code just the unit test and a high-level description of the goal).

stuartmorgan avatar Mar 11 '24 16:03 stuartmorgan

@stuartmorgan Thanks for your reminder.

I have adjusted the implementation to solve this issue by removing the cyclic object, please review the code again.

LinXunFeng avatar Mar 12 '24 11:03 LinXunFeng

Unless I'm missing something, the new version is destroying any duplicate objects, which is not the same thing as just removing cycles.

stuartmorgan avatar Mar 12 '24 15:03 stuartmorgan

@stuartmorgan I've re-adjusted the removal logic.

LinXunFeng avatar Mar 15 '24 10:03 LinXunFeng

cc ~~@hellohuanlin~~ @cbracken to review

jmagman avatar Apr 10 '24 20:04 jmagman