feat: Refactor data source connection handling. #591
Conversation
…build' into rlamb/sdk-195/support-js-style-initialization
…build' into rlamb/sdk-195/support-js-style-initialization
| * Note: The requestor is implemented independently from polling such that it can be used to | ||
| * make a one-off request. | ||
| * | ||
| * @internal |
There was a problem hiding this comment.
To allow usage outside the common package.
packages/sdk/browser/jest.config.js
Outdated
| }, | ||
| testPathIgnorePatterns: ['./dist'], | ||
| testPathIgnorePatterns: ['./dist', './src'], | ||
| testMatch: ["**.test.ts"] |
There was a problem hiding this comment.
The shared test code didn't contain tests, which made it angry.
| "scripts": { | ||
| "clean": "rimraf dist", | ||
| "build": "rollup -c rollup.config.js", | ||
| "build": "tsc --noEmit && rollup -c rollup.config.js", |
There was a problem hiding this comment.
Make sure TS errors actually fail the build.
| // TODO: Handle wait for network results in a meaningful way. SDK-707 | ||
|
|
||
| try { | ||
| const payload = await requestor.requestPayload(); |
There was a problem hiding this comment.
The current SDK only does this once. We may want to consider if we want to at least do it twice.
| const plainContextString = JSON.stringify(Context.toLDContext(context)); | ||
| const requestor = this.getRequestor(plainContextString); | ||
|
|
||
| // TODO: Handle wait for network results in a meaningful way. SDK-707 |
There was a problem hiding this comment.
Maybe it just isn't an option for this SDK?
| const uri = getPollingUri(this.config.serviceEndpoints, path, parameters); | ||
| return new Requestor(this.platform.requests, uri, headers, method, body); | ||
| } | ||
| // TODO: Automatically start streaming if event handlers are registered. |
There was a problem hiding this comment.
Made a ticket for this. Was originally in scope for this ticket, but it already ended up being too large.
| flagManager: FlagManager, | ||
| configuration: Configuration, | ||
| baseHeaders: LDHeaders, | ||
| emitter: LDEmitter, |
There was a problem hiding this comment.
Consider emitter -> errorEmitter.
There was a problem hiding this comment.
The emitter is not error only, but all emissions. Though this does only use it in that domain. It doesn't need to. I could make an emit error function if that makes more sense.
There was a problem hiding this comment.
Basically DataManager only emits errors in the implementations we have, but it could emit any supported event type. Though maybe we don't want that? If we didn't then we would need to pass something other than an emitter, as an emitter lets you emit any event type.
| undefined, | ||
| diagnosticsManager, | ||
| start, | ||
| false, |
There was a problem hiding this comment.
Is this start case covered by a call to setEventSendingEnabled?
There was a problem hiding this comment.
Yes. It makes it easier to control from the specific SDK implementations. RN conditionally starts it and the Browser SDK always starts it.
kinyoklion
deleted the
rlamb/sdk-195/support-js-style-initialization
branch
Most of this PR is refactoring to move data source handling our of the common code. This also moves functionality, such as
setConnectionModeout of the common code.Currently the MobileDataManager is in the RN SDK, but it should be moved to some kind of non-browser common when we implement SDKs like node or electron.
A large amount of the code in this PR is tests for data source handling and moving existing code.
This PR does not add automatic starting/stopping of the stream.
BEGIN_COMMIT_OVERRIDE
feat: Refactor data source connection handling.
feat: Add support for js-client-sdk style initialization.
END_COMMIT_OVERRIDE