#1 Work on initial scaffold. #4

kinyoklion · 2022-05-19T23:38:17Z

The goal of this is to provide a starting point for the LDClient to start working out from. I also moved the platform implementation into a sub-directory.

While working through the interfaces though I realized a bit of a problem. That is that the EventEmitter type was on the LDClient as well as the BigSegmentStoreStatusProvider. The EventEmitter is a node specific type, so I would prefer not to have it in the common interface.

So, in order to get around this I removed it from the common interface, then provided extended interfaces in the node platform. So the node platform interface will look the same as it did before, but the common code will not include the EventEmitter.

To allow for this I incorporate the common client into the platform through composition, versus extending it with the node items. So the node client acts as a facade/proxy for the common client. It also has a facade around the big segement store status provider to allow it to emit events the node way.

If we were to write a different environment platform we would likely do almost the same thing, but use EventTarget instead of EventEmitter.

Newer nodes have EventTarget support, so that may be something that could be consolidated in the future. There is a frankenstein type that implements EventTarget and EventEmitter, called NodeEventTarget. It could be used to allow a graceful deprecation of sorts.

Nothing here does anything really. Just some plumbing code.

I also took a stab at a top level export. But a lot will change there once I make an init method.

shortcut-integration · 2022-05-19T23:38:20Z

This pull request has been linked to Shortcut Story #151493: Create placeholder classes/implementations that satisfy the public interface..

kinyoklion · 2022-05-19T23:38:47Z

platform-node/src/BigSegmentsStoreStatusProviderNode.ts

+import EventEmitter from 'events';
+import { BigSegmentStoreStatusProvider } from './api/interfaces/BigSegmentStoreStatusProvider';
+
+export default class BigSegmentStoreStatusProviderNode


This provides EventEmitter support for the BigSegmentStoreStatus.

I don't currently have a better idea, and this is probably something that already occurred to you, but... is it not possible to have the EventEmitter be a separate object, rather than merging its behavior into these platform-specific components? In other words, have there be just one implementation of BigSegmentStoreStatusProvider, but it contains an EventEmitter object— whatever implementation is provided by the platform— as a private property, and delegates the event-related methods to that.

In my imagination at least, that seems cleaner than requiring the platform code to delegate all the non-platform-specific methods to an underlying component.

At the moment I am trying to keep it compatible with the existing interface, which is that the object itself directly emits events. Which is not the most ideal interface, so potentially I should just change it.

I did consider providing the type for the emitter to the base, and I may explore this further. We could potentially just make the type generic over an emitter. Though we would still need something that provided a consistent API.

We could use a method from one of the existing emitter types EventEmitter and EventTarget. In which case only one of them would require a small wrapper. Basically just adding the method to emit and event and calling the platform specific variation.

LDClientImpl<EventEmitter>
Then carry that through to other components like the big segment status.

It does require implementing a couple of methods of course, but the code involved is minimal, I would think... and you are already doing something like that in your current code, you are just doing it the other way round (having to delegate all the component's methods that are not the event methods).

Is this an argument at a conceptual level, that you prefer I proxy event emitter than LDClient?

Event emitter has 14 methods. It isn't just a couple. Which means 28 methods if I apply it to segment status and this. I might be able to make a mixin that only requires writing the proxy once. But I want to be clear it isn't a couple.

Updated using a mixin which allows for the types to be extended with an event emitter. Basically changes the has an emitter composition into is an emitter.

Ugh, I am clearly out of date on that API. I was hoping it was fewer.

I do think that for internal components at least, we should be able to write against a simpler interface. I mean, we can determine which methods we actually need to use. For public-facing components like BigSegmentStoreStatusProvider, that doesn't apply, and if you really feel that doing it the way you're doing it here is the least worst option then OK. I am just unhappy about the idea of reimplementing components like this in the platform-specific modules that do not have any platform-specific logic in them other than the event emitter; it feels backwards to me and also means there is more stuff for us to maintain in the platform-specific code.

Well, I have updates it now with the mixin. It does prevent the re-implemetation, and it allows the event emitter implementation to be shared.

It is more complex though, and causes some type complexity. I am willing to give this approach a try for now. The implementation in the platform should not often change much, especially the events, so it may be somewhat complex, but it should be unchanging.

If we don't like it down the road, then I can adjust.

kinyoklion · 2022-05-19T23:41:17Z

server-sdk-common/src/BigSegmentStatusProvider.ts

@@ -0,0 +1,36 @@
+/* eslint-disable class-methods-use-this */


This will have to hook into some big segment management. But I created it so I could put something in the interface to allow it to build and be relatively intact.

kinyoklion · 2022-05-19T23:42:32Z

server-sdk-common/src/api/LDClient.ts

   */
  on(event: string | symbol, listener: (...args: any[]) => void): this;
-
-  // The following are symbols that LDClient inherits from EventEmitter, which we are declaring


I do not have this at the node level at the moment. I will need to experiment with doc generation, but I will likely come to the same conclusion.

kinyoklion · 2022-05-23T17:26:49Z

platform-node/src/LDClientNode.ts

+  }
+}
+
+export default Emits(LDClientNode);


When we actually create one of these, then we will need to return an interface. This will currently act as a value instead of a type, which we do not want.

kinyoklion · 2022-05-23T17:27:20Z

platform-node/src/Emits.ts

@@ -0,0 +1,80 @@
+import EventEmitter from 'events';


The implementation here allows you to 'mix' an emitter into any type which contains an emitter.

kinyoklion · 2022-05-23T17:29:55Z

platform-node/src/Emits.ts

+import EventEmitter from 'events';
+
+export type EventableConstructor<T = {}> = new (...args: any[]) => T;
+export type Eventable = EventableConstructor<{ emitter: EventEmitter }>;


For more information you can look here: https://www.typescriptlang.org/docs/handbook/mixins.html

This is the constrained variety: https://www.typescriptlang.org/docs/handbook/mixins.html#constrained-mixins

kinyoklion · 2022-05-23T17:38:30Z

platform-node/src/api/LDClient.ts

@@ -0,0 +1,22 @@
+import { LDClient as LDClientCommon } from '@launchdarkly/js-server-sdk-common';


When we add an init method the LDClient interfaces exported by this class is the one we will want to use. This will hide the complexity of our internal type implementation and mixins.

kinyoklion · 2022-05-23T17:40:09Z

server-sdk-common/src/LDClientImpl.ts

+  Failed,
+}
+
+export default class LDClientImpl implements LDClient {


I may rename this 'Impl' classes to common or something later. 'Impl' feels off.

The typescript gods decided interfaces do not start with I so naming becomes harder.

kinyoklion added 2 commits May 19, 2022 16:26

Work on initial scaffold.

a090170

Update index exports.

0b470ea

kinyoklion commented May 19, 2022

View reviewed changes

Add an extra blank line.

93fbac6

kinyoklion commented May 19, 2022

View reviewed changes

kinyoklion marked this pull request as ready for review May 19, 2022 23:42

kinyoklion requested review from aengelberg, eli-darkly and keelerm84 May 19, 2022 23:42

kinyoklion added 3 commits May 23, 2022 10:20

Attempt using mixin for event emitter.

8be08da

Add implementation for emitting events for big segment status.

5302683

Add documentation.

263fccd

kinyoklion commented May 23, 2022

View reviewed changes

Fix lint and interface hierarchy.

831b964

kinyoklion commented May 23, 2022

View reviewed changes

kinyoklion changed the title ~~Work on initial scaffold.~~ #1 Work on initial scaffold. May 24, 2022

kinyoklion mentioned this pull request May 24, 2022

#2 Implement options parsing. #5

Merged

keelerm84 approved these changes May 25, 2022

View reviewed changes

kinyoklion merged commit a47fa26 into main May 25, 2022

kinyoklion deleted the rlamb/sc-151493/ldclient-scaffold branch May 25, 2022 17:24

kinyoklion added a commit that referenced this pull request Aug 23, 2022

#4 Add tests for using a proxy. (#38)

7274899

		@@ -0,0 +1,22 @@
		import { LDClient as LDClientCommon } from '@launchdarkly/js-server-sdk-common';

#1 Work on initial scaffold. #4

#1 Work on initial scaffold. #4

Uh oh!

Conversation

kinyoklion commented May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shortcut-integration bot commented May 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kinyoklion commented May 19, 2022 •

edited

Loading