-
Notifications
You must be signed in to change notification settings - Fork 31
[sc-151494] Add platform interfaces and initial implementation. #3
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.
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
Conversation
| ignorePatterns: ["**/dist/**"], | ||
| rules: { | ||
| }, | ||
| ignorePatterns: ["**/dist/**"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was duplicated.
| @@ -0,0 +1,6 @@ | |||
| module.exports = { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we use typescript jest needs a config file to understand how to compile things.
| @@ -0,0 +1,120 @@ | |||
| import * as http from 'http'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This covers the basics of requests. I will see about a standalone TLS and Proxy test later.
| "version": "1.0.0", | ||
| "description": "Node platform for the JS Server SDK", | ||
| "type": "commonjs", | ||
| "main": "./dist/index.js", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some changes to structure to allow package.json to be imported. The typescript compiler doesn't like things outside the rootdir.
| @@ -0,0 +1,15 @@ | |||
| /* eslint-disable class-methods-use-this */ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Crypto and Filesystem are just pass through implementations of the node versions of these methods.
| @@ -0,0 +1,102 @@ | |||
| // import { Options, platform, options } from '@launchdarkly/js-server-sdk-common'; | |||
| import createHttpsProxyAgent, { HttpsProxyAgentOptions } from 'https-proxy-agent'; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This package does not look abandoned. Unlike tunnel which we were using. It should provide the same functionality. I need to add some testing for it later.
| return { | ||
| ca: tlsOptions.ca, | ||
| cert: tlsOptions.cert, | ||
| checkServerIdentity: tlsOptions.checkServerIdentity, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This checkServerIdentity function does not look to have been implemented correctly in the node server SDK before. The issue was it was being passed as a request parameter. All the other TLS parameters can be passed in a request, but this is not part of that interface. I did some testing to validate that it does not get called when used like that.
It does work correctly when incorporated into an agent.
That creates overlap between the proxy and TLS, both of which are agents. Thankfully node-https-proxy forwards additional configuration to the underlying agent so we can easily combine the two.
| /** | ||
| * @inheritdoc | ||
| */ | ||
| fetch(url: string, options: platform.Options = {}): Promise<platform.Response> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to go with a fetch interface here because it should be more portable in the long-run. It is available in the various web worker and wasm environments. Additionally node is adding its own fetch support eventually.
| @@ -1,3 +1,24 @@ | |||
| import doesItWork from '@launchdarkly/js-server-sdk-common'; | |||
| /* eslint-disable no-console */ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file contains temporary code at the moment. A convenient place to try things.
|
|
||
| /** | ||
| * Allows you to specify a host for an optional HTTP proxy. | ||
| * Allows you to specify configuration for an optional HTTP proxy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change. So I could consider introducing these at the top level again and then doing some config work to put them into an object. This does seem like a cleaner way to expose them in the interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could release a transitional minor version where both are supported but the top-level ones are deprecated.
eli-darkly
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very clear and sensible.
Co-authored-by: Eli Bishop <eli@launchdarkly.com>
Co-authored-by: Alex Engelberg <alex.benjamin.engelberg@gmail.com> Co-authored-by: Matthew M. Keeler <keelerm84@gmail.com>
…through. (#14) * Add bucketing and common tests. * Correct spelling. * Check numeric conversion step. * Add operators and tests. * Add unrecognized operator test. * Add interfaces for the data we use during evaluations. Start scaffolding evaluator. * Cleanup * Add target evaluation. * Add documentation and tests. * Linting. * Add parsers and update evaluation types. * Add TODO comment and lint. * Add internal specifiers. * Improve implementation and add tests. * Comment updates. * Helps if you save. * Add prerequisite evaluations. * Refine prerequisite code. * Progress on rule evaluation. * Implement rules and fallthrough. * Add clause tests and fix bugs. * Add rules tests. * Remove TODO that is no longer needed. * Rename static methods to lowercase. * Rename matchClause.
The primary goal of this PR was to add the platform interfaces as well as an initial implementation.
In doing so I actually started using the other interfaces, so I needed to setup a structure to export them. So a good amount of code shows as changed from things being re-arranged.