search

metarhia / jstp

Fast RPC for browser and Node.js based on TCP, WebSocket, and MDSF
https://metarhia.github.io/jstp
Other
142 stars 10 forks source link

lib: add support for async SessionStorageProvider #323

Closed belochub closed 6 years ago

belochub commented 6 years ago

Ping, @nechaido, @lundibundi.

belochub commented 6 years ago

@lundibundi, I don't quite understand how it is possible to tell the difference between requested session being nonexistent and SessionStorageProvider being asynchronous in your proposed solution because in both of this situations return value will be undefined. Also, there must be no need for removing the isAsync method in the future, because we still must allow using synchronous SessionStorageProvider.

nechaido commented 6 years ago

@lundibundi it is a bad practice to have one function that passes return value into callback or returns it depending on the arguments.

nechaido commented 6 years ago

I'd prefer to make callback mandatory, but I'm ok with the current implementation.

lundibundi commented 6 years ago

@nechaido I know that, but I assumed that we wanted to make it mandatory in the future and that code was a necessary evil for compatibility and would have been removed at some point in the future. @belochub But if that's not the case and we will allow both ways then yeah, the current implementation is obviously better and I'm fine with it. Also, @belochub yeah, that's a good point, I missed that, well, as there is already an unsightly code for compatibility we could've said that async providers have to return null and not undefined upon async usage.

belochub commented 6 years ago

Landed in https://github.com/metarhia/jstp/commit/a5b7927df0956f613e18b41f998628563a219fe7.