KO Reactor / KO Undo Manager compatibility (context is not a function)

[silviuburceadev]

I am not sure where this issue belongs/must be fixed, so I'm opening in both repositories, since @bago is KO Reactor contributor and KO Undo Manager author.

According to KO Reactor (link), it supports a context option, which is undocumented. Looking at its usages in assignWatcher function, context should be a function.

However, KO Undo Manager is building an empty object (link) as context and is passing it to KO Reactor (link).

This leads to a TypeError: context is not a function error.

At this point, there are 2 options:

• KO Undo Manager passing an empty function instead of empty object, which is what KO Reactor expects
• KO Reactor checking if context is a function before executing it, e.g. typeof context === 'function' && context(returnValue) instead of context(returnValue).

Additionally, it might help if KO Reactor's context option is documented to indicate how it should look like.

I am happy to contribute with a PR if needed, I'm just not sure what's the correct way to handle this. Both options would be backwards-compatible.

[bago]

Hi I'm not maintaining knockout-undomanager anymore as I didn't record "users" for that library and our project mosaico was the only downstream user I was aware of, so I simply moved the code and development to mosaico.

The code you're talking about has not been changed in mosaico but I'm sure it works with reactor.

I'm almost sure I don't see that "context is not a function error" so I guess the way Mosaico uses the undomanager and reactor doesn't trigger the issue.

I think the first thing to do is to create a test case to prove the issue. I'm sure the undomanager had issues inside mosaico when I didn't pass the empty object as context, but I really don't remember what issue. Running a search in ko-reactor issues I found this one posted by me: https://github.com/ZiadJ/knockoutjs-reactor/issues/25 https://github.com/ZiadJ/knockoutjs-reactor/issues/25#issuecomment-137504654

[silviuburceadev]

Well, I don't know what I'm missing here but this source code (link) screams at me that context must be a function, isn't it?

I think you should try to update knockout-undomanager with the improvements you've done for mosaico and release it on NPM (fixing #4 too), I promise you it is used in the wild and NPM would allow you to see weekly downloads, too. KO Reactor should probably check if context is a function (and maybe rename it to its callback as it looks like one).

I'll try to come up with a test case too, in our legacy project we're using Undo Manager for an observable array of objects (with Knockout Mapping).

[bago]

You can see in the issue I linked above that I had the same doubts about "context" and given context is not documented I can only stand to ZiadJ words in https://github.com/ZiadJ/knockoutjs-reactor/issues/25#issuecomment-137504654 "any object would do as context" as valid. It is working in my use case (I confirm that call has not been changed in mosaico local fork and it works fine even after updating to the latest reactor.