kvlt
kvlt copied to clipboard
nervous-systems
make EventSource optional
When using kvlt on the Microsoft Edge browser, the compiled javascript fails because EventSource is not supported and the code tries to 'require' the eventsource module (presumably for node.js). When removing the SSE support in a fork (we don't actually use it in our code), the problem no longer appears. A clean way to turn off the SSE support would allow us avoid maintaining a fork.
I've noticed that you have created a (possibly?) similar ticket for WebSocket (#20). If you have ideas on how you see options for turning on/off various features, we'd be happy to try to create a PR for this.
@loomis So I included both of those features (SSE and websockets), in retrospect, maybe a little exuberantly, despite not using them myself. There is a snapshot (0.2.0-SNAPSHOT) and corresponding branch (wip/smaller) which just removes that functionality and updates the deps. I'm tempted to release the snapshot at some point - would that break anything for you?
If so, (delay)'ing the requires and derefing them on first use may be the simplest approach.
@moea Just creating a release without those features would currently work for us. However, we're actually planning to start using websockets in the next few weeks, so that would be a very short term fix for us. I'll look to see if the delays would be easy to add.
@moea Unfortunately, it doesn't look like any solution with delay will work for clojurescript. The first issue is that require can only appear at the top-level in clojurescript. Therefore, it can't appear in a delay and necessitates that the user explicitly require the kvlt.platform.* namespaces. Even with that, clojurescript doesn't have any of the resolve-like functions, so the correct request! function can't be looked up dynamically (at least not without a lot of direct hacking with javascript).
The only moderately clean solution that I can find is to separate the websocket! and event-source! functions into their own namespaces. Users would then be able to choose which features they need, but with the inconvenience of requiring more than just the current kvlt.core namespace.
Would you be open to a split-namespace solution? Or perhaps you can think of something I've missed with delayed loading in clojurescript.
@loomis Ah, sorry, I wasn't being clear - I meant delaying the calls to js/require in the cljs modules, e.g:
(def websocket (delay (js/require "websocket"))
And then explicitly deref'ing the websocket var within that module, at the point of use - everything would appear fine on platforms where the import will be unsuccessful, up until the point where the functionality is exercised.
(the only caveat would be if there are module-level uses of the required Node modules, though if there are, wrapping them in functions or just delaying them, also, shouldn't be too much trouble).
If you're going to tackle this, I am totally fine with you removing the SSE functionality and just delaying the websocket stuff, or that's something I can do subsequently - I think it's a valuable change.
@moea I've generated a PR (#26) with changes to delay the loading of EventSource and WebSocket.
I've added some logging to the delay to give the user some more specific feedback when either library is unavailable; the default "Uncaught ReferenceError: require is not defined" is not very helpful in understanding the problem. Currently the code re-throws the exception, but could be changed to eat the exception or generate one with a different method.
Getting the constructor to work from the delayed value was a bit difficult. In the end I had to bind a local symbol to get the constructor syntax to work. There's some magic going on there, that I don't understand and didn't have time to dig into. If you know a better incantation that doesn't require the local binding, please let me know.
The modified library works for us, even with the Edge browser. I've seen that the CI tests pass as well. I haven't done much other testing, so you might want to take it for a real-world spin before merging it.