add warning to multiple sdk instances with the same parameters #566
Conversation
| type Signature = AppManifest['signature'] | ||
| type ActyxHost = ActyxOpts['actyxHost'] | ||
| type ActyxPort = ActyxOpts['actyxPort'] | ||
| type Param = [AppId, Version, Signature, ActyxHost, ActyxPort] |
There was a problem hiding this comment.
I dislike the Param name, Params/Parameters makes much more sense given it's a list.
I'd also add a brief docstring
There was a problem hiding this comment.
these types aren’t visible to anyone, so a docstring won’t be displayed anywhere — did you mean a comment?
No. This is purely an internal logging mechanism opaque to the user |
| const oldDispose = actyx.dispose | ||
| actyx.dispose = () => { | ||
| const result = oldDispose() | ||
| actyxInstanceRegister.subOne(manifest, opts) |
There was a problem hiding this comment.
Does oldDispose() invoke some user callbacks? If so, such callback could try to create a new SDK instance, which in that case would log the “multiple connections” error. If correctness permits it, I’d do the subOne before the oldDispose.
There was a problem hiding this comment.
It only calls MultiplexedWebsocket.dispose under the hood.
There should be no difference between both orders since this is a sync function.
There was a problem hiding this comment.
My worry was that there might be an event emitted to user code from that sync function — and if we add such a thing in the future then this problem may well surface.
There was a problem hiding this comment.
Ah, that's a valid concern
| type Signature = AppManifest['signature'] | ||
| type ActyxHost = ActyxOpts['actyxHost'] | ||
| type ActyxPort = ActyxOpts['actyxPort'] | ||
| type Param = [AppId, Version, Signature, ActyxHost, ActyxPort] |
There was a problem hiding this comment.
these types aren’t visible to anyone, so a docstring won’t be displayed anywhere — did you mean a comment?
Co-authored-by: Roland Kuhn <[email protected]>
No description provided.