knockout-postbox
knockout-postbox copied to clipboard
rniemeyer
When publishing a value with a circular reference, a 'cyclic object value' error is thrown
When publishing a value with a circular reference, a 'cyclic object value' error is thrown. This is because of the serialization that happens to fill topicCache.
Code to reproduce:
const foo = {
id: "foo",
bar: ko.observable()
}
const bar = {
id: "bar",
foo: ko.observable()
}
ko.postbox.subscribe("foo", function() {
console.log("foo was published");
})
ko.postbox.publish("foo", foo); // console: foo was published
foo.bar(bar);
ko.postbox.publish("foo", foo); // console: foo was published
bar.foo(foo);
ko.postbox.publish("foo", foo); // TypeError: cyclic object value
Is this something that can be solved by the library, or is it something I need to solve in my code?
I'm currently using this workaround:
const serializer = ko.postbox.serializer;
ko.postbox.serializer = (object: any) => { return undefined; }
ko.postbox.publish("selectAdministration", this.administration);
ko.postbox.serializer = serializer;
I'm not using subscribeTo(...), or subscribe(...) with initializeWithLatestValue set to true, so I don't think I'm breaking anything.
As you can see in this fiddle, the exception has nothing to do with knockout.postbox but with serialization.
https://jsfiddle.net/qqqy2q43/
My opinion is to fix it in your code as the only way to "fix" this is to replace/remove serialization as you are already doing. Luckily postbox allows you to replace the serialization, which defaults to ko.toJSON() but as you can see in my fiddle the bug exists aswel in JSON.stringify().
Btw, ko.toJSON() actually uses JSON.stringify() as you can see here:
https://github.com/knockout/knockout/blob/master/src/subscribables/mappingHelpers.js#L20 https://github.com/knockout/knockout/blob/master/src/utils.js#L534
This kind of code is going to throw an exception in alot of libraries around there tho. Looking at the above links I would even guess knockout itself is not going to like this at some point.
Note: I Got nothing to do with this library, just my 2 cents :-D
As you can see in this fiddle, the exception has nothing to do with knockout.postbox but with serialization.
I'm aware of the technical details and understand them, but one could argue that the serialization happening here is an implementation detail of knockout-postbox, since I had to dig into the code to understand where the error came from.
The best thing to do in order to understand a library is always to dig in the code, so no issue there tbh.
The serialization happening here is totally unrelated from postbox tho, as you can tell by looking at the source code. One could argue that a library, which extends knockout, should be using the same serialization as knockout itself, which postbox is doing.
So ye, on that part one could argue that knockout could be fixing this. But then again I guess you will be pointed to the JS specs as JSON serialization doesn't support circular references (but hey, why would it need to serialize the exact same 2 objects over and over again ?).
On the other hand, I wouldn't realy want a library to support your situation for serialization as you have a circular reference which would lead to a never ending (and useless) serialization.
If you look at http://stackoverflow.com/questions/10392293/stringify-convert-to-json-a-javascript-object-with-circular-reference they have a workaround which involves adapting the serialization in such a way that it excludes the circular references. Imho this is a lot of effort for something that could have been avoided by not passing in an object with circular referencing.
Not that it's my business, but why are you using this circular references ?
Not that it's my business, but why are you using this circular references ?
We have a UI built with components (hence the use of knockout-postbox, for communication between them), where certain actions can link two ~~viewmodels~~ data models together and then they sometimes need to access each other's data. There are likely other ways to tackle this without circular references, but this seemed like the easiest and most logical implementation to us.
Using circular references will most likely never be the easiest solution in a language where serializing those objects is not supported.
On Sep 1, 2016 17:02, "Stijn Herreman" [email protected] wrote:
Not that it's my business, but why are you using this circular references ?
We have a UI built with components (hence the use of knockout-postbox, for communication between them), where certain actions can link two viewmodels together and then they sometimes need to access each other's data. There are likely other ways to tackle this without circular references, but this seemed like the easiest and most logical implementation to us.
— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/rniemeyer/knockout-postbox/issues/42#issuecomment-244107420, or mute the thread https://github.com/notifications/unsubscribe-auth/ACDCV0uV74qJMNsyxD-12gWzrJlyxmJjks5qluj0gaJpZM4Jyl3a .
Btw u are using postbox for losely coupling up components yet u bind up view models directly to each other. Imho not a good idea as u are breaking loosely coupling.
Not rly related but I just wanted to mention it.
Aparantly I can't edit my posts on mobile, so sorry for the amount of replies.
Imho if u want a more clean solution then urs I would go for :
- create a ko extension method subscribeWithoutSerialization which allows u for reusing it and even sharing it with the community.
- create a library (e. g. Json.parse.circular.js) and override json stringify and parse to handle ur case. But u can never entirely serialize or deserialize circular references without stopping the circular referencing at some lever. Ur stack will explode if u try to
I would go for PR with a change like this:
exports.defaultCacheInitializer = function(value) {
return {
value: value,
serialized: exports.serializer(value)
};
}
exports.cacheInitializer = exports.defaultCacheInitializer;
//wrap notifySubscribers passing topic first and caching latest value
exports.publish = function(topic, value) {
if (topic) {
//keep the value and a serialized version for comparison
exports.topicCache[topic] = exports.cacheInitializer(value);
exports.notifySubscribers(value, topic);
}
};
So you can just get rid of all the serialization for initializing and comparing cache values if it does no suit your needs.
By the way, knockout.postbox is amazing, thanks for that.
That might be a bit cleaner then overriding the serializer but the effect is comparable to how @stijnherreman is overriding the serializer ?
I think that in practice the effect is the same, using the defaults will still throw the serialization error and to avoid it you need to override something. But it will make the library not dependent in serialization anymore, as it is only related to the default cache comparison, and it could be replaced with something else, like timestamps or ids.
Also, it could make easier to create cache related extensions to ko.postbox.
Well, something came up now. The serialization concept (cached.serialized specifically) could be change to something like a 'hash' or 'memento' or 'identifier'. I mean, something that could be used to compare values like a Object#equals in java. Thinking about it it is used only as a optimization to avoid running serialized(cached.value) over and over.
Might not be interesting, but when I started to use this library in the past days I simply assigned the serializer an identity function like
ko.postbox.serializer = function(value) { return value };
because I actually don't need the caching and the notification with latest value feature currently.
Well eventually you might hit a point where you do need to serialize the object which includes a circular reference, and removing the serializer won't help in that situation.
If that's the case, you may want to look into Douglas Crockford's cycle.js: https://github.com/douglascrockford/JSON-js
For KO postbox, you could easily create a wrapper around cycle.js and use it to serialize and deserialize your objects containing circular references.