#2 Implement options parsing. #5

kinyoklion · 2022-05-23T23:45:49Z

This PR adds configuration parsing and validation.

While the SDK is typescript consumers may be using JS, or they may just force TS to their will. So we need to carefully validate anything that comes from an external source.

This adds most of the bast configuration and tests. Additional configuration will be added later as additional components are implemented.

#4 is the base for this PR.

The PR is pretty long, but that is largely from tests. There isn't a large amount of logic, but I wanted to test configuration items relatively independently. So it will be easy to adjust individual option configuration later.

shortcut-integration · 2022-05-23T23:45:51Z

This pull request has been linked to Shortcut Story #154352: Implement options class and parsing..

kinyoklion · 2022-05-23T23:46:34Z

platform-node/tsconfig.json

    "declaration": true,
    "declarationMap": true, // enables importers to jump to source
    "resolveJsonModule": true,
+    "stripInternal": true


This is to support @internal. So we can implement things and keep their details out of our exported types.

kinyoklion · 2022-05-23T23:48:07Z

server-sdk-common/src/options/Configuration.ts

+// types. Calls to the SDK could contain anything without any regard to typing.
+// So, data we take from external sources must be normalized into something
+// that can be trusted.
+


Typescript basically can verify your code, that you write, at compile time. Once it is compiled, then anyone can do anything with it. Many people may use the SDK from JS and they will just pass us any random object.

So this converts that object into something that does meet the typings and then we can use further without additional runtime checks (mostly).

kinyoklion · 2022-05-23T23:49:40Z

server-sdk-common/src/options/validators.ts

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


This is vaguely inspired by: https://github.com/gcanti/io-ts/blob/master/index.md

I would rather not include any dependencies beyond what are strictly required. Especially for common, which we want to run on anything without worrying about polyfills.

kinyoklion · 2022-05-24T16:40:10Z

server-sdk-common/src/options/validators.ts

+    this.typeOf = typeof example;
+  }
+
+  is(u: unknown): u is T {


Type guards are special methods. We aren't really using the benefit they provide here, but I figured that we may as well provide it in case we need it later.

If you use a type guard for an if-statement, then the contents of that statement are treated as the guarding type.

class Cat { meow() {} } function isCat(value: any): value is Cat { return ("meow" in value) && (typeof value.meow === 'function'); } function example() { const fish = { meow: () => {} }; if(isCat(fish)) { // We have informed the type system that this fish is indeed a cat, so it will not complain. fish.meow() } }

kinyoklion · 2022-05-24T19:34:07Z

jest.config.js

@@ -1,5 +1,11 @@
 module.exports = {
  transform: {'^.+\\.ts?$': 'ts-jest'},


Some updates to make coverage more useful.

kinyoklion · 2022-05-24T19:35:17Z

server-sdk-common/__tests__/Logger.ts

@@ -0,0 +1,62 @@
+import { LDLogger } from '../src';


This is a logger implementation for tests. Helps to validate some common scenarios.

aengelberg

This is great! I read through and it overall makes sense to me (a non-TypeScript-expert).

aengelberg · 2022-05-24T20:51:44Z

server-sdk-common/__tests__/options/Configuration.test.ts

+        value < 60
+          ? OptionMessages.optionBelowMinimum('diagnosticRecordingInterval', value as number, 60)
+          : OptionMessages.wrongOptionType('diagnosticRecordingInterval', 'number with minimum value of 60', typeof value),
+      );


Because all your examples expect either 0 or 1 warning, you could potentially add more detail to the "test data" (i.e. your list of tuples) indicating what warning you expect to see, if any, and then simplify the logic of the assertions later on. One way I've seen this done is to use regexes to confirm that certain phrases appear in the warning:

it.each([ [0, 60, /using minimum of 60/], [500, 500, null], ['potato', 900, /should be of type number with minimum value of 60/], ])('allow setting and validates diagnosticRecordingInterval', (value, expected, warningPattern) => { const config = new Configuration(withLogger({ diagnosticRecordingInterval: value })); expect(config.diagnosticRecordingInterval).toEqual(expected); // This is probably DRY-able if (warningPattern) { expect(logger(config).warningMessages.length).toEqual(1); expect(logger(config).warningMessages[0]).toMatch(warningPattern); } else { expect(logger(config).warningMessages.length).toEqual(0); } }

I have a thought here. I can probably just add a containsMessages or something like that to the TestLogger.
I only validated the specific message on a couple, because most are the same, but I could at least make it cleaner.

aengelberg · 2022-05-24T21:45:35Z

server-sdk-common/src/options/Configuration.ts

+      validatedOptions.streamUri,
+      validatedOptions.baseUri,
+      validatedOptions.eventsUri,
+    );


FYI, ever since we added the new ServiceEndpoints configuration builder to Java, Go, and .NET, those SDKs now have some additional validation to detect when, for example,

the caller has overridden the streaming URI to a custom value, but not polling or events

the SDK is configured to send events (i.e. sendEvents = true)

and then we log a warning about it, because that almost definitely means they missed some configuration.

Even though we don't have a ServiceEndpoints "builder" here, and we don't really need one, it might still be worth bringing the level of validation up to parity so it can also catch those cases. I think that can be added fairly easily now that we have a dedicated "config validation" phase.

Yeah, I was thinking about that and just kind of forgot about it. Can add a message for it though. Thanks for bringing it up.

…thodology for logs.

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

server-sdk-common/src/options/Configuration.ts

+  return { errors, validatedOptions };
+}
+
+function validateEndpoints(options: LDOptions, validatedOptions: ValidatedOptions) {


I am doing this up-front for now. But once the other components are implemented I may move it. If it looks like it will be cleaner.

keelerm84 · 2022-05-25T17:21:47Z

server-sdk-common/src/options/Configuration.ts

+  Object.keys(options).forEach((optionName) => {
+    // We need to tell typescript it doesn't actually know what options are.
+    // If we don't then it complains we are doing crazy things with it.
+    const optionValue = (options as unknown as any)[optionName];


kinyoklion · 2022-05-25T17:28:59Z

sdk-common/src/Context.ts

@@ -0,0 +1,19 @@
+type MultiKind = {


This guy isn't intentional, but doesn't hurt anything. In progress stuff upcoming.

aengelberg

I like the improvements. I had some minor comments about how the endpoint validation logic could be more consistent, and an observation about the new testing code.

server-sdk-common/src/options/Configuration.ts

aengelberg · 2022-05-25T17:41:29Z

server-sdk-common/src/options/Configuration.ts

+    validatedOptions.logger?.warn(OptionMessages.partialEndpoint('streamUri'));
+  }
+
+  if (!pollingEndpointSpecified) {


Likewise, for polling

Suggested change

if (!pollingEndpointSpecified) {

if (!pollingEndpointSpecified && !validatedOptions.stream) {

aengelberg · 2022-05-25T17:43:01Z

server-sdk-common/src/options/Configuration.ts

+  const { baseUri, streamUri, eventsUri } = options;
+  const streamingEndpointSpecified = streamUri !== undefined && streamUri !== null;
+  const pollingEndpointSpecified = baseUri !== undefined && baseUri !== null;
+  const eventEndpointSpecified = eventsUri !== undefined && eventsUri !== null;


If you want to be consistent with how the other SDKs do it, I believe these checks would be "is the URI set to some non-null value that is not equal to the default URI". i.e. if it's not null, but happens to be equal to the default, that doesn't count as "custom" for the purposes of this check.

aengelberg · 2022-05-25T17:47:30Z

server-sdk-common/__tests__/options/Configuration.test.ts

+    expect(logger(config).getCount()).toEqual(logs.length);
+    // There should not be any messages, so checking them for undefined is a workaround
+    // for a lack of pure assert.
+    logger(config).verifyMessages(logs).forEach((message) => expect(message).toBeUndefined());


The fact that verifyMessages returns the list of missing messages threw me at first when reading this, because that was the opposite of what I assumed was happening. Also, this .forEach makes me wonder if there's a way to avoid putting in this addendum every time.

In Jest, can you call expect during a helper function, outside of the scope of a unit test? It might be cleaner if this method were called expectMessages(logs) and took care of all of the expecting.

I will think about the missing versus not. I generally try to avoid any assertions outside the test file itself. You can do it, but then the stacks and things are not very apparent what is happening.

Maybe to clean up the "undefined" situation, the verify messages function could return null if everything's ok. Then this could be

expect(logger(config).verifyMessages(logs)).toBeNull();

Co-authored-by: Alex Engelberg <alex.benjamin.engelberg@gmail.com>

kinyoklion added 13 commits May 19, 2022 16:26

Work on initial scaffold.

a090170

Update index exports.

0b470ea

Add an extra blank line.

93fbac6

Attempt using mixin for event emitter.

8be08da

Add implementation for emitting events for big segment status.

5302683

Add documentation.

263fccd

Fix lint and interface hierarchy.

831b964

Start implementation.

3b1ece5

Progress on option handling.

cec9858

Add application tags support.

954a135

Un-generalize tags.

1a0dffc

Fix tag formatting.

a141aeb

Comments.

40d27e3

kinyoklion changed the title ~~Start implementation.~~ Implement options parsing. May 23, 2022

kinyoklion commented May 23, 2022

View reviewed changes

Add internal to validated options.

f429571

kinyoklion commented May 23, 2022

View reviewed changes

Add tests for type checks. Finish implementing type checks.

5792122

kinyoklion changed the title ~~Implement options parsing.~~ #2 Implement options parsing. May 24, 2022

kinyoklion commented May 24, 2022

View reviewed changes

kinyoklion added 6 commits May 24, 2022 09:54

Add remaining validator tests.

b93ed50

Improve coverage collection. Start implementing application tags tests.

82599e2

Add test logger and finish tag tests.

ef67115

Start testing the Configuration class.

cd8357a

Fix validation. Start on individual option tests.

7ea76b5

Add remaining config tests.

82788fd

kinyoklion commented May 24, 2022

View reviewed changes

kinyoklion requested a review from aengelberg May 24, 2022 19:34

kinyoklion requested a review from keelerm84 May 24, 2022 19:34

kinyoklion marked this pull request as ready for review May 24, 2022 19:34

kinyoklion commented May 24, 2022

View reviewed changes

Make sure to check resolves in the logger.

9740584

aengelberg approved these changes May 24, 2022

View reviewed changes

Implement check for service endpoints consistency. Improve testing me…

b573151

…thodology for logs.

kinyoklion requested a review from aengelberg May 25, 2022 17:22

kinyoklion commented May 25, 2022

View reviewed changes

Base automatically changed from rlamb/sc-151493/ldclient-scaffold to main May 25, 2022 17:24

keelerm84 approved these changes May 25, 2022

View reviewed changes

Merge branch 'main' into rlamb/sc-154352/options-parsing

4283204

kinyoklion commented May 25, 2022

View reviewed changes

Remove unintended file.

9080329

aengelberg approved these changes May 25, 2022

View reviewed changes

kinyoklion and others added 2 commits May 25, 2022 11:04

Update server-sdk-common/src/options/Configuration.ts

5e0d9b1

Co-authored-by: Alex Engelberg <alex.benjamin.engelberg@gmail.com>

Refactor expectMessages.

79dd5d6

kinyoklion merged commit 3f079e7 into main May 25, 2022

kinyoklion deleted the rlamb/sc-154352/options-parsing branch May 25, 2022 18:25

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

		@@ -1,5 +1,11 @@
		module.exports = {
		transform: {'^.+\\.ts?$': 'ts-jest'},

	if (!pollingEndpointSpecified) {
	if (!pollingEndpointSpecified && !validatedOptions.stream) {

#2 Implement options parsing. #5

#2 Implement options parsing. #5

Uh oh!

Conversation

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

Uh oh!

Uh oh!

shortcut-integration bot commented May 23, 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!

aengelberg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

aengelberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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 23, 2022 •

edited

Loading

aengelberg left a comment •

edited

Loading

aengelberg May 25, 2022 •

edited

Loading