Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Add platform support for async hashing. #573

Merged
merged 13 commits into from
Sep 9, 2024
Merged

feat: Add platform support for async hashing. #573

merged 13 commits into from
Sep 9, 2024

Conversation

kinyoklion
Copy link
Member

@kinyoklion kinyoklion commented Sep 6, 2024 •
edited
Loading

This adds platform support for async hashing for use in client-side SDKs.

It does not implement async hashing for any existing platform, but provides it as an option to allow for use of standard browser APIs. Allowing the usage of standard browser crypto APIs means that browser SDKs will not need to include an additional dependency to replicate built-in functionality.

@kinyoklion kinyoklion force-pushed the rlamb/add-platform-support-for-async-hashing branch from 472b0f1 to 365f048 Compare September 6, 2024 19:55
@kinyoklion kinyoklion changed the title feat: Add platform support for async hasing. feat: Add platform support for async hashing. Sep 6, 2024
@kinyoklion kinyoklion force-pushed the rlamb/add-platform-support-for-async-hashing branch from 365f048 to c4a67ce Compare September 6, 2024 19:56
@kinyoklion kinyoklion closed this Sep 6, 2024
@kinyoklion kinyoklion reopened this Sep 6, 2024
kinyoklion
kinyoklion commented Sep 6, 2024
View reviewed changes
packages/shared/common/src/api/platform/Crypto.ts
/**
* Note: Server SDKs MUST implement createHmac.
*/
createHmac?(algorithm: string, key: string): Hmac;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could create Hmac on all platforms, but we don't actually need to.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically I would prefer implementation of this not block or slow down platform implementations, and I would like for SDKs to not have unused bloat to implement it.

kinyoklion
kinyoklion commented Sep 6, 2024
View reviewed changes
packages/shared/common/src/api/platform/Crypto.ts
@@ -7,7 +7,19 @@
*/
export interface Hasher {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a little clunky. It would be nice if as much was statically checked as possible, but the platforms are not likely to be wrong outside initial implementation. We do have a couple other dynamic elements.

kinyoklion
kinyoklion commented Sep 6, 2024
View reviewed changes
packages/shared/sdk-client/src/flag-manager/FlagManager.ts
this.flagUpdater = new FlagUpdater(this.flagStore, logger);
this.flagPersistence = new FlagPersistence(
this.flagPersistencePromise = this.initPersistence(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assign the promise synchronously and then await it when persistence needs to be used.

Getting the promise immediately removes any race conditions you would have with code that awaits and assigns the value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is async now as generating the hash can be async.

@kinyoklion kinyoklion marked this pull request as ready for review September 6, 2024 22:38
@kinyoklion kinyoklion requested a review from a team as a code owner September 6, 2024 22:38
tanderson-ld
tanderson-ld reviewed Sep 9, 2024
View reviewed changes
packages/shared/sdk-client/src/crypto/digest.ts Outdated Show resolved Hide resolved
packages/shared/sdk-server/src/BigSegmentsManager.ts
if (!hasher.digest) {
// This represents an error in platform implementation.
throw new Error('Platform must implement digest or asyncDigest');
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a straightforward way to make a default digest implementation that throws the error to centralize this case? I suppose that would advocate for hasher.digest and hasher.asyncDigest becoming non-optional with default erroring implementations.

Copy link
Member Author

@kinyoklion kinyoklion Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do that, then you cannot choose one to call without throwing an exception. Also, unlike Java for instance, there are not checked exceptions. The optional typing forces you do make a decision because the compiler won't let it be unhandled, so you have to check it, optionally call it, or force it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be centralized similar to the client, but it didn't seem worth it for the limited call sites, and that it is just a pass-through call, versus the tiny bit of logic required in the client.

@kinyoklion kinyoklion merged commit 9248035 into main Sep 9, 2024
22 checks passed
@kinyoklion kinyoklion deleted the rlamb/add-platform-support-for-async-hashing branch September 9, 2024 20:06
@github-actions github-actions bot mentioned this pull request Sep 10, 2024
Merged
kinyoklion pushed a commit that referenced this pull request Sep 26, 2024
@github-actions
chore: release main (#572)
dc4eb30 
🤖 I have created a release *beep* *boop*
---


<details><summary>akamai-edgeworker-sdk-common: 1.2.0</summary>

##
[1.2.0](akamai-edgeworker-sdk-common-v1.1.15...akamai-edgeworker-sdk-common-v1.2.0)
(2024-09-26)


### Features

* Add support for conditional event source capabilities.
([#577](#577))
([fe82500](fe82500))


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @launchdarkly/js-server-sdk-common bumped from ^2.6.1 to ^2.7.0
</details>

<details><summary>akamai-server-base-sdk: 2.1.16</summary>

##
[2.1.16](akamai-server-base-sdk-v2.1.15...akamai-server-base-sdk-v2.1.16)
(2024-09-26)


### Dependencies

* The following workspace dependencies were updated
  * dependencies
* @launchdarkly/akamai-edgeworker-sdk-common bumped from ^1.1.15 to
^1.2.0
    * @launchdarkly/js-server-sdk-common bumped from ^2.6.1 to ^2.7.0
</details>

<details><summary>akamai-server-edgekv-sdk: 1.1.16</summary>

##
[1.1.16](akamai-server-edgekv-sdk-v1.1.15...akamai-server-edgekv-sdk-v1.1.16)
(2024-09-26)


### Dependencies

* The following workspace dependencies were updated
  * dependencies
* @launchdarkly/akamai-edgeworker-sdk-common bumped from ^1.1.15 to
^1.2.0
    * @launchdarkly/js-server-sdk-common bumped from ^2.6.1 to ^2.7.0
</details>

<details><summary>cloudflare-server-sdk: 2.5.14</summary>

##
[2.5.14](cloudflare-server-sdk-v2.5.13...cloudflare-server-sdk-v2.5.14)
(2024-09-26)


### Dependencies

* The following workspace dependencies were updated
  * devDependencies
    * @launchdarkly/js-server-sdk-common-edge bumped from 2.3.9 to 2.4.0
</details>

<details><summary>js-client-sdk-common: 1.8.0</summary>

##
[1.8.0](js-client-sdk-common-v1.7.0...js-client-sdk-common-v1.8.0)
(2024-09-26)


### Features

* Add platform support for async hashing.
([#573](#573))
([9248035](9248035))
* Add support for conditional event source capabilities.
([#577](#577))
([fe82500](fe82500))
* Add support for js-client-sdk style initialization.
([53f5bb8](53f5bb8))
* Add URLs for custom events and URL filtering.
([#587](#587))
([7131e69](7131e69))
* Adds support for REPORT.
([#575](#575))
([916b724](916b724))
* Allow using custom user-agent name.
([#580](#580))
([ed5a206](ed5a206))
* Implement goals for client-side SDKs.
([#585](#585))
([fd38a8f](fd38a8f))
* Refactor data source connection handling.
([53f5bb8](53f5bb8))


### Bug Fixes

* Flag store should not access values from prototype.
([#567](#567))
([fca4d92](fca4d92))
* Use flag value whenever provided even if variaiton is null or
undefined. ([#581](#581))
([d11224c](d11224c))


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @launchdarkly/js-sdk-common bumped from 2.8.0 to 2.9.0
</details>

<details><summary>js-sdk-common: 2.9.0</summary>

##
[2.9.0](js-sdk-common-v2.8.0...js-sdk-common-v2.9.0)
(2024-09-26)


### Features

* Add platform support for async hashing.
([#573](#573))
([9248035](9248035))
* Add support for conditional event source capabilities.
([#577](#577))
([fe82500](fe82500))
* Add URLs for custom events and URL filtering.
([#587](#587))
([7131e69](7131e69))
* Adds support for REPORT.
([#575](#575))
([916b724](916b724))
* Allow using custom user-agent name.
([#580](#580))
([ed5a206](ed5a206))
* Implement goals for client-side SDKs.
([#585](#585))
([fd38a8f](fd38a8f))


### Bug Fixes

* Multi-kind context containing only 1 kind conveted incorrectly.
([#594](#594))
([b6ff2a6](b6ff2a6))
</details>

<details><summary>js-server-sdk-common: 2.7.0</summary>

##
[2.7.0](js-server-sdk-common-v2.6.1...js-server-sdk-common-v2.7.0)
(2024-09-26)


### Features

* Add platform support for async hashing.
([#573](#573))
([9248035](9248035))
* Add support for conditional event source capabilities.
([#577](#577))
([fe82500](fe82500))
* Allow using custom user-agent name.
([#580](#580))
([ed5a206](ed5a206))


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @launchdarkly/js-sdk-common bumped from 2.8.0 to 2.9.0
</details>

<details><summary>js-server-sdk-common-edge: 2.4.0</summary>

##
[2.4.0](js-server-sdk-common-edge-v2.3.9...js-server-sdk-common-edge-v2.4.0)
(2024-09-26)


### Features

* Add support for conditional event source capabilities.
([#577](#577))
([fe82500](fe82500))


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @launchdarkly/js-server-sdk-common bumped from 2.6.1 to 2.7.0
</details>

<details><summary>node-server-sdk: 9.6.0</summary>

##
[9.6.0](node-server-sdk-v9.5.4...node-server-sdk-v9.6.0)
(2024-09-26)


### Features

* Add support for conditional event source capabilities.
([#577](#577))
([fe82500](fe82500))


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @launchdarkly/js-server-sdk-common bumped from 2.6.1 to 2.7.0
</details>

<details><summary>node-server-sdk-dynamodb: 6.1.22</summary>

##
[6.1.22](node-server-sdk-dynamodb-v6.1.21...node-server-sdk-dynamodb-v6.1.22)
(2024-09-26)


### Dependencies

* The following workspace dependencies were updated
  * devDependencies
    * @launchdarkly/node-server-sdk bumped from 9.5.4 to 9.6.0
  * peerDependencies
    * @launchdarkly/node-server-sdk bumped from >=9.4.3 to >=9.6.0
</details>

<details><summary>node-server-sdk-otel: 1.0.14</summary>

##
[1.0.14](node-server-sdk-otel-v1.0.13...node-server-sdk-otel-v1.0.14)
(2024-09-26)


### Dependencies

* The following workspace dependencies were updated
  * devDependencies
    * @launchdarkly/node-server-sdk bumped from 9.5.4 to 9.6.0
  * peerDependencies
    * @launchdarkly/node-server-sdk bumped from >=9.4.3 to >=9.6.0
</details>

<details><summary>node-server-sdk-redis: 4.1.22</summary>

##
[4.1.22](node-server-sdk-redis-v4.1.21...node-server-sdk-redis-v4.1.22)
(2024-09-26)


### Dependencies

* The following workspace dependencies were updated
  * devDependencies
    * @launchdarkly/node-server-sdk bumped from 9.5.4 to 9.6.0
  * peerDependencies
    * @launchdarkly/node-server-sdk bumped from >=9.4.3 to >=9.6.0
</details>

<details><summary>react-native-client-sdk: 10.7.0</summary>

##
[10.7.0](react-native-client-sdk-v10.6.1...react-native-client-sdk-v10.7.0)
(2024-09-26)


### Features

* Add support for conditional event source capabilities.
([#577](#577))
([fe82500](fe82500))
* Add support for js-client-sdk style initialization.
([53f5bb8](53f5bb8))
* Adds support for REPORT.
([#575](#575))
([916b724](916b724))
* Refactor data source connection handling.
([53f5bb8](53f5bb8))


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @launchdarkly/js-client-sdk-common bumped from 1.7.0 to 1.8.0
</details>

<details><summary>vercel-server-sdk: 1.3.17</summary>

##
[1.3.17](vercel-server-sdk-v1.3.16...vercel-server-sdk-v1.3.17)
(2024-09-26)


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @launchdarkly/js-server-sdk-common-edge bumped from 2.3.9 to 2.4.0
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tanderson-ld tanderson-ld tanderson-ld approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kinyoklion @tanderson-ld