Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Broadcasting of requested resolution #8

Merged
merged 8 commits into from
Jan 8, 2021
Merged

Broadcasting of requested resolution #8

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
855d61a
allow broadcasting of requested resolution
owenpearson Jan 5, 2021
ef5d5aa
allow updating of requested resolution
owenpearson Jan 5, 2021
a34aa8a
pass resolution to AssetConnection and remember on change request
owenpearson Jan 6, 2021
ef95d1b
raise exception when sendChangeRequest is called without subscription
owenpearson Jan 6, 2021
b1da3ed
execute user provided callbacks asynchronously
owenpearson Jan 7, 2021
4a06271
use promises for resolution change requests
owenpearson Jan 7, 2021
9abeb51
use PascalCase for enum members (as per ts docs)
owenpearson Jan 8, 2021
7b9bf35
add comment to explain nextTick/setImmediate
owenpearson Jan 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 31 additions & 12 deletions src/lib/AssetConnection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,12 @@
import Ably, { Types as AblyTypes } from 'ably';
import { LocationListener, StatusListener } from '../types';
import { ClientTypes, LocationListener, Resolution, StatusListener } from '../types';
import Logger from './utils/Logger';

enum EventNames {
raw = 'raw',
enhanced = 'enhanced',
}

enum ClientTypes {
subscriber = 'subscriber',
publisher = 'publisher',
}

class AssetConnection {
logger: Logger;
ably: AblyTypes.RealtimePromise;
Expand All @@ -20,20 +15,23 @@ class AssetConnection {
onRawLocationUpdate?: LocationListener;
onEnhancedLocationUpdate?: LocationListener;
onStatusUpdate?: StatusListener;
resolution: Resolution | null;

constructor(
logger: Logger,
trackingId: string,
ablyOptions: AblyTypes.ClientOptions,
onRawLocationUpdate?: LocationListener,
onEnhancedLocationUpdate?: LocationListener,
onStatusUpdate?: StatusListener
onStatusUpdate?: StatusListener,
resolution?: Resolution
) {
this.logger = logger;
this.trackingId = trackingId;
this.onRawLocationUpdate = onRawLocationUpdate;
this.onEnhancedLocationUpdate = onEnhancedLocationUpdate;
this.onStatusUpdate = onStatusUpdate;
this.resolution = resolution ?? null;

this.ably = new Ably.Realtime.Promise(ablyOptions);
this.channel = this.ably.channels.get(trackingId, {
Expand All @@ -57,6 +55,22 @@ class AssetConnection {
this.ably.close();
};

performChangeResolution = async (
resolution: Resolution,
onSuccess: () => unknown,
onError: (err: Error) => unknown
): Promise<void> => {
try {
await this.channel.presence.update({
type: ClientTypes.subscriber,
resolution,
});
onSuccess();
} catch (e) {
onError(e);
}
};

private subscribeForRawEvents = (rawLocationListener: LocationListener) => {
this.channel.subscribe(EventNames.raw, (message) => {
const parsedMessage = typeof message.data === 'string' ? JSON.parse(message.data) : message.data;
Expand All @@ -81,17 +95,22 @@ class AssetConnection {

private joinChannelPresence = async () => {
this.channel.presence.subscribe(this.onPresenceMessage);
this.channel.presence.enterClient(this.ably.auth.clientId, ClientTypes.subscriber).catch((reason) => {
this.logger.logError(`Error entering channel presence: ${reason}`);
throw new Error(reason);
});
this.channel.presence
.enterClient(this.ably.auth.clientId, {
type: ClientTypes.subscriber,
resolution: this.resolution,
})
.catch((reason) => {
this.logger.logError(`Error entering channel presence: ${reason}`);
throw new Error(reason);
});
};

private leaveChannelPresence = async () => {
this.channel.presence.unsubscribe();
this.notifyAssetIsOffline();
try {
await this.channel.presence.leaveClient(this.ably.auth.clientId, ClientTypes.subscriber);
await this.channel.presence.leaveClient(this.ably.auth.clientId);
} catch (e) {
this.logger.logError(`Error leaving channel presence: ${e.reason}`);
throw new Error(e.reason);
Expand Down
17 changes: 15 additions & 2 deletions src/lib/AssetSubscriber.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Types as AblyTypes } from 'ably';
import { LocationListener, StatusListener } from '../types';
import { LocationListener, Resolution, StatusListener } from '../types';
import AssetConnection from './AssetConnection';
import Logger, { LoggerOptions } from './utils/Logger';

Expand All @@ -9,6 +9,7 @@ export type SubscriberOptions = {
onEnhancedLocationUpdate?: LocationListener;
onStatusUpdate?: StatusListener;
loggerOptions?: LoggerOptions;
resolution?: Resolution;
};

class AssetSubscriber {
Expand All @@ -18,13 +19,15 @@ class AssetSubscriber {
onEnhancedLocationUpdate?: LocationListener;
logger: Logger;
assetConnection?: AssetConnection;
resolution?: Resolution;
QuintinWillison marked this conversation as resolved.
Show resolved Hide resolved

constructor(options: SubscriberOptions) {
this.logger = new Logger(options.loggerOptions);
this.ablyOptions = options.ablyOptions;
this.onStatusUpdate = options.onStatusUpdate;
this.onRawLocationUpdate = options.onRawLocationUpdate;
this.onEnhancedLocationUpdate = options.onEnhancedLocationUpdate;
this.resolution = options.resolution;
}

start(trackingId: string): void {
Expand All @@ -34,10 +37,20 @@ class AssetSubscriber {
this.ablyOptions,
this.onRawLocationUpdate,
this.onEnhancedLocationUpdate,
this.onStatusUpdate
this.onStatusUpdate,
this.resolution
);
}

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.'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds reasonable to me - I've fixed this and other instances where we call user provided callbacks in b1da3ed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does ably-js do? (cc @SimonWoolf )

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

@SimonWoolf SimonWoolf Jan 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

} else {
this.assetConnection.performChangeResolution(resolution, onSuccess, onError);
}
}

stop = async (): Promise<void> => {
await this.assetConnection?.close?.();
delete this.assetConnection;
Expand Down
19 changes: 19 additions & 0 deletions src/types/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,22 @@ export type GeoJsonMessage = unknown;
export type LocationListener = (geoJsonMsg: GeoJsonMessage) => unknown;

export type StatusListener = (isOnline: boolean) => unknown;

export enum Accuracy {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were you going to address this comment in the scope of this PR? It felt like you said you might elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, forgot to do that - done now in 9abeb51.

minimum = 1,
low = 2,
balanced = 3,
high = 4,
maximum = 5,
}

export type Resolution = {
accuracy: Accuracy;
desiredInterval: number;
minimumDisplacement: number;
};

export enum ClientTypes {
subscriber = 'subscriber',
publisher = 'publisher',
}