Custom subscription actions

[theY4Kman]

This PR is intended to supersede #11, and builds atop the wonderful (and for many months, thankless) contributions from @jhillacre

Fixes #10
Closes #9

Brief

This PR adds support for custom subscription actions through .subscribe(), and changes SubscriptionPromise.cancel(), .close(), and .unsubscribeAll() to return Promises that resolve upon completion.

Also, this PR switches logging from loglevel to winston, because it offered the configurability I wasn't finding from loglevel.

Description

subscribe()

To support custom subscription actions, the prototype for subscribe() has been modified. It now accepts an options object, allowing subscribeAction: 'my_custom_subscribe' and unsubscribeAction: 'my_custom_unsubscribe' to be passed. includeCreateEvents: true may also be passed in options, instructing the client to also listen for "action": "create" events over the wire. Additionally, an arbitrary object may be passed in place of pk, enabling custom arguments to be passed to the action.

/**
 * Subscribe to update and delete events for an object, or perform a custom subscription
 *
 * @param stream Name of object's type stream
 * @param args Identifying information to be included in subscription request
 * @param callback Function to call with payload on new events
 * @param options Optional object to configure the subscription
 * @param options.requestId Specific request ID to submit with the
 *    subscription/unsubscription request, and which will be included in
 *    responses from DCRF. If not specified, one will be automatically generated.
 * @param options.subscribeAction Name of action used in subscription request.
 *    By default, 'subscribe_instance' is used.
 * @param options.unsubscribeAction Name of action used in unsubscription request.
 *    By default, 'unsubscribe_instance' is used.
 * @param options.includeCreateEvents Whether to listen for creation events,
 *    in addition to updates and deletes. By default, this is false.
 * @return Promise Resolves/rejects when response to subscription request received.
 *    On success, the promise will be resolved with null, or an empty object.
 *    On failure, the promise will be rejected with the entire API response.
 *    This Promise has an additional method, cancel(), which can be called
 *    to cancel the subscription.
 */
subscribe(stream: string, args: object, callback: SubscriptionHandler, options?: SubscribeOptions): CancelablePromise<object | null>;

SubscriptionPromise.cancel() / close() / unsubscribeAll()

These three methods have been changed to return Promises that resolve when their actions are completed.

Many moons ago, DCRF (or CRF?) didn't support unsubscribing, so no requests were sent to the server upon canceling a subscription — the client would simply ignore those unsubscribed events. However — also many moons ago 😅 — DCRF added support for unsubscribing, so there grew a good reason for dcrf-client to wait for those unsubscription requests to complete. @jhillacre converted SubscriptionPromise.cancel() to a Promise in #11 to cover that case.

In this PR, I've also converted client.close() and client.unsubscribeAll() to return Promises — though, my main motivation for this was to ensure all subscriptions from one test were cleaned up before the next one ran.

Logging

I believe the initial reason for switching from loglevel to winston was loglevel's lack of argument formatting — there was one place I was doing something like log.debug('Thing is: %s', thing) and expecting Thing is: <thing>, but actually getting This is: %s <Thing>.

Upon switching, I enjoyed the option of including colours to the console, which really helped when debugging the integration tests.

However, it should be noted that I haven't inspected what the logs look like in a browser! It may spew too much by default... Or it may be printing nothing. It might be worth adding some ability to configure the log levels to dcrf-client.

Miscellaneous

TypeScript was upgraded to 3.7+ to gain support for optional chaining (a?.b?.c instead of a && a.b && a.b.c) and nullish coalescing (a ?? b instead of a == null ? b : a), because I believe it makes the code more succinct and easier to read.

[theY4Kman]

@jhillacre @pmack — I'd love to know what y'all think of this version in practice. Besides the includeCreateEvents, it doesn't seem like a huge lift, but I'm a little concerned about possible annoyances (or shortcomings) with the logging from winston.

With your blessing, though, I'll have this pushed up to npm in the blink of an eye.

Although we kept the arguments for subscribe() backwards-compatible, the return types for those few public functions were changed, and by strict semantic versioning rules, a major version bump is warranted. It's a shame I'm not using this opportunity to move the callback/handler arg to the back of the prototype, where it belongs :P

// Current prototype without chopping down the args
client.subscribe('streeheem', {arg: 1}, (obj, action) => {
	do.a.thing(obj, action);
}, {
	subscribeAction: 'smash_that_subscribe',
	unsubscribeAction: 'vanish',
});

//  I don't like the lack of continuation indents


// Current prototype with chopped args
client.subscribe(
	'streeheem', 
	{arg: 1}, 
	(obj, action) => {
		do.a.thing(obj, action);
	}, 
	{
		subscribeAction: 'smash_that_subscribe',
		unsubscribeAction: 'vanish',
	},
);

// Feels a bit overly spaced... 
// and, of course, the options arg seems out of order —
// if the handler is long, the options might be overlooked.
//
// It shows that my decision to have an optional requestId param
// at the end of the method really wasn't thought through. It really
// ought to have been an `options` obj from the start.


// I wonder if it may make sense to have `options` and `handler` mutually exclusive,
// such that if `options` are specified, `handler` is one of the properties
client.subscribe('streeheem', {arg: 1}, {
	subscribeAction: 'smash_that_subscribe',
	unsubscribeAction: 'vanish',
	handler(obj, action) {
		do.a.thing(obj, action);
	}
});

Just thinkin :P

[theY4Kman]

I just added a few more commits to resolve those testing woes from #10. Now, in this PR:

• The README says to use pipenv install --dev to get the deps
• The npm run test:integration now performs pipenv run py.test
• The Python version has been bumped to 3.8 to resolve the weird importlib_metadata error (which I was able to reproduce when using pipenv's auto-created virtualenv)

[jhillacre]

Handler as option property or as options argument seems reasonable to me.

Although we kept the arguments for subscribe() backwards-compatible, the return types for those few public functions were changed, and by strict semantic versioning rules, a major version bump is warranted. It's a shame I'm not using this opportunity to move the callback/handler arg to the back of the prototype, where it belongs :P

It's early in the project, more major bumps now are good. I always say we aren't running out of version numbers. One thing to help users coming for the journey might be a CHANGES.md, detailing breaking changes.

I just added a few more commits to resolve those testing woes from #10. Now, in this PR:

• The README says to use pipenv install --dev to get the deps
• The npm run test:integration now performs pipenv run py.test
• The Python version has been bumped to 3.8 to resolve the weird importlib_metadata error (which I was able to reproduce > * when using pipenv's auto-created virtualenv)

I can confirm that the tests work for me on the new branch. 👍👍

TypeScript was upgraded to 3.7+ to gain support for optional chaining (a?.b?.c instead of a && a.b && a.b.c) and nullish coalescing (a ?? b instead of a == null ? b : a), because I believe it makes the code more succinct and easier to read.

I needed to blow away my old node_modules to get the build to work. Seemed like the old 'tsc' is used during 'prepare' instead of the new one, but I wasn't able to reproduce by manually installing old 'typescript' versions then reinstalling using npm install.

src/index.ts:70:37 - error TS1109: Expression expected.

70     this.pkField = options.pkField ?? 'pk';
                                       ~

src/index.ts:70:43 - error TS1005: ':' expected.

70     this.pkField = options.pkField ?? 'pk';

[theY4Kman]

Well, I guess I'll save a prototype for the next major version, just to get this out the door quicker.

And good call on a changelog. It shows how long ago I released this project... all my new things start with a CHANGELOG.md. I'll add one now.

[theY4Kman]

@jhillacre Alright, version 1.0.0 has been published! https://www.npmjs.com/package/dcrf-client/v/1.0.0

Thanks for all your work, and for spending your own time to update the code and open a PR for it. It's much appreciated, and deserved a much swifter response than you got. Now, let's see if we can't get that pagination going...

With all the stuff I've had to pull to get the integration tests robust, it seems some PRs to DCRF itself are in order for me.