feat: cloudflare sdk base #74
Conversation
…tore and configuration to ts.
|
This pull request has been linked to Shortcut Story #195479: Move cloudflare sdk to js-core. |
yusinto
changed the title
There was a problem hiding this comment.
I'm exporting this so the cloudflare unit tests can use async/await when testing feature store functionality. It's much better to use async/await for unit tests than promise thenables.
|
|
||
| const defaults = { | ||
| stream: false, | ||
| sendEvents: false, |
There was a problem hiding this comment.
Are we keeping this for now because that's how it was in the original SDK? For that SDK it was set to false because it wasn't technically possible to polyfill the API we were using.
There was a problem hiding this comment.
I tried sendEvents: true In #76 and ran miniflare tests. The tests pass but jest exited disgracefully (no offence jest):
Jest did not exit one second after the test run has completed.
'This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue.
Is this needed for go live on Apr 28? I'm focusing on feature parity and there's still other chores outstanding like documentation, releasing and CI.
There was a problem hiding this comment.
For parity no, and Tim C should probably be consulted before a default change to the behavior is implemented.
There was a problem hiding this comment.
Yep. This will be a major release though so changing sendEvents to true shouldn't be an issue. It does make sense to sendEvents and it's extremely valuable.
I will attempt to include this for go-live only if other necessary tasks are done. If not we can always release a minor version to support this. I'm sure there are many other improvements in addition to this after go-live.
| @@ -0,0 +1,33 @@ | |||
| // TODO: Dry this file duplicated in this repo | |||
There was a problem hiding this comment.
This duplication has been fixed in #75
There was a problem hiding this comment.
If we are using this at runtime we may want to call it NullEventSource. Just because that is a convention we have used for Noop type components in other SDKs. NullEventProcessor for an example.
Not critical, just a thought, because mock often invokes testing thoughts.
There was a problem hiding this comment.
Ok done in #75 which deals with making it shareable.
….com:launchdarkly/js-core into yus/sc-195479/move-cloudflare-sdk-to-js-core
* chore: added ci config * fix: typedoc error index.ts not included in tsconfig.
* chore: added miniflare dep and initial test file. * fix: suppress node warnings and enable modules when running jest * chore: added miniflare tests * chore: turned on jest coverage flag * chore: add prettierignore --------- Co-authored-by: Yusinto Ngadiman <yus@launchdarkly.com>
* chore: moved eventSource for sharing * chore: renamed MockEventSource NullEventSource
This is the base branch for the cloudflare sdk. Please review this PR first before the following child prs: