Broadcasting of requested resolution #8
Conversation
| @@ -3,3 +3,22 @@ export type GeoJsonMessage = unknown; | |||
| export type LocationListener = (geoJsonMsg: GeoJsonMessage) => unknown; | |||
|
|
|||
| export type StatusListener = (isOnline: boolean) => unknown; | |||
|
|
|||
| export enum Accuracy { | |||
There was a problem hiding this comment.
Take a brief look around the place in terms of TypeScript enum definitions, some places seem to prefer CamelCase. Equally I would have expected this to be picked up by the linter and/or prettier if it's non-standard and that doesn't seem to be the case. Is there a convention we're aligning with here?
There was a problem hiding this comment.
I am aware that camelCase is often used for enums but I'm aligning with the TypeScript documentation which to me feels more like the right thing to do.
There was a problem hiding this comment.
Were you going to address this comment in the scope of this PR? It felt like you said you might elsewhere.
There was a problem hiding this comment.
Sorry, forgot to do that - done now in 9abeb51.
owenpearson
force-pushed
the
feature/requested-resolution
branch
from
71c466b to
a34aa8a
Compare
src/lib/AssetSubscriber.ts
Outdated
| sendChangeRequest(resolution: Resolution, onSuccess: () => unknown, onError: (err: Error) => unknown): void { | ||
| this.resolution = resolution; | ||
| if (!this.assetConnection) { | ||
| onError(new Error('Cannot change resolution; no asset is currently being tracked.')); |
There was a problem hiding this comment.
Thanks for making this change, but this now brings up another issue for me...
By calling onError in the way you are now, you are making it a blocking call - as in the app's onError implementation must finish executing (return) before this "method" can return.
My expectation would be that a less surprising result for app developers would be for you to delay the call to onError until after this method has returned. I believe that can be accomplished using setTimeout.
Are there any other locations within this codebase where we have methods that accept callback functions and end up calling them synchronously, as I would imagine those should change too (assuming you agree with me).
There was a problem hiding this comment.
Sounds reasonable to me - I've fixed this and other instances where we call user provided callbacks in b1da3ed.
There was a problem hiding this comment.
What does ably-js do? (cc @SimonWoolf )
There was a problem hiding this comment.
ably-js does appear to call callbacks synchronously in some error cases. I agree it wouldn't be a bad idea to chuck in a setImmediate for those. But no-one's complained yet so probably not a huge deal in practice
There was a problem hiding this comment.
@SimonWoolf My question was more in relation to the other issue, which is what do we think is the appropriate best practice for setTimeout/setImmediate/nextTick, and how to make it portable?
There was a problem hiding this comment.
ably-js has Utils.nextTick which does setTimeout(f, 0) on browsers (which is what owen's done here) and process.nextTick in node.
AIUI node's setImmediate is basically just an alias for setTimeout(f, 0), ie adds it as a timer action in the next eventloop. Vs process.nextTick which despite the name requeues the action in the current event loop at the end of the queue. For delaying a callback, 'end of current event' loop is arguably semantically neater, but there's no way to do that in browsers afaik, and for that purpose isn't going to make a difference.
(for batch methods that want to move things to the next event loop the difference starts to matter - nextTick is not sufficient, as we've discovered - but that's not relevant to this)
There was a problem hiding this comment.
I'm happy to approve this for today, in order to get our next preview release out, but I would appreciate it if you could ensure that any deferred fixes are either raised immediately as new PRs or have issues created for them so they don't get forgotten. Thanks.
src/lib/utils/utils.ts
Outdated
| @@ -0,0 +1,3 @@ | |||
| export const nextTick = (fn: () => unknown): void => { | |||
There was a problem hiding this comment.
Thanks for adding this, however...
Feel free to defer it to another PR, but this would be a perfect opportunity to add some commentary to this method to explain what its purpose is. We know right now, looking at it from the context of this PR, but later on the meaning will become less clear to those browsing the library.
Also, while I can imagine your reasons for using these words to form this function name, it's not a very obvious name. "tick" is an overloaded word at the best of times, while nextTick as a function name suggests it might be some kind of "getter", not a "doer". Something like emitSoon might be better, perhaps?
There was a problem hiding this comment.
I chose nextTick since it's effectively the same thing as Node's process.nextTick. I've changed it to setImmediate in 7b9bf35 since that's what Simon called it and is also the name of a non-standard browser global that does the same thing. I also added a comment to explain what it's there for.
| @@ -3,3 +3,22 @@ export type GeoJsonMessage = unknown; | |||
| export type LocationListener = (geoJsonMsg: GeoJsonMessage) => unknown; | |||
|
|
|||
| export type StatusListener = (isOnline: boolean) => unknown; | |||
|
|
|||
| export enum Accuracy { | |||
There was a problem hiding this comment.
Were you going to address this comment in the scope of this PR? It felt like you said you might elsewhere.
owenpearson
force-pushed
the
feature/requested-resolution
branch
from
153dae7 to
7b9bf35
Compare
If merged, this PR will allow broadcasting of requested resolution in channel presence data via a configuration option, and updating requested resolution from a new public API method,
sendChangeRequest.