Skip to content

Conversation

@kinyoklion
Copy link
Member

@kinyoklion kinyoklion commented Mar 18, 2024

Adds hook support.

More than 50% of the line count is unit testing and extending contract test support. I could potentially re-organize to simplify the review if needed.

@kinyoklion kinyoklion force-pushed the rlamb/implement-hook-support branch 3 times, most recently from 3fcfd6d to 12e6126 Compare March 18, 2024 23:01
hasEventListeners: () => boolean;
}

const BOOL_VARIATION_METHOD_NAME = 'LDClient.boolVariation';
Copy link
Member Author

Choose a reason for hiding this comment

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

An alternative to this would be to do something like LDClient${this.boolVariation.name} within each method. Doing that would potentially produce lots of temporary variables, so instead a constant is defined for each method.


private featureStore: LDFeatureStore;

private asyncFeatureStore: AsyncStoreFacade;
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 was dead code left over after the change to callbacks.

@kinyoklion kinyoklion force-pushed the rlamb/implement-hook-support branch from 12e6126 to 0a63276 Compare March 18, 2024 23:08
methodName: string,
method: () => Promise<LDEvaluationDetail>,
): Promise<LDEvaluationDetail> {
if(this.hooks.length == 0) {
Copy link
Member Author

Choose a reason for hiding this comment

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

When there are no hooks we just return the non-awaited method, which should yield similar performance to the current implementation.

@kinyoklion kinyoklion force-pushed the rlamb/implement-hook-support branch 2 times, most recently from 6477c77 to 830a22e Compare March 18, 2024 23:37
@kinyoklion kinyoklion force-pushed the rlamb/implement-hook-support branch from 830a22e to 34e1971 Compare March 18, 2024 23:41
@kinyoklion kinyoklion marked this pull request as ready for review March 19, 2024 16:38
@yusinto
Copy link
Contributor

yusinto commented Mar 19, 2024

The PR size is fine to me. Thank you for your kind consideration though, appreciate that.

@kinyoklion kinyoklion requested a review from keelerm84 March 29, 2024 18:52
);
});

it('executes hook stages in the specified order', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Should we have a test verifying the order between config and client hooks? In ruby I did config then client.

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'll add a runtime registered hook. I don't think I need another test. It is "registered order" and "reverse registered order" So if I register a hook C, it would execute after a and b for before and then before, b and a for after.

@kinyoklion
Copy link
Member Author

I think I can move the hook execution code out of the client, but I would like to do that in a discreet PR.

@kinyoklion kinyoklion merged commit 669acb8 into feat/hooks Apr 1, 2024
@kinyoklion kinyoklion deleted the rlamb/implement-hook-support branch April 1, 2024 21:51
kinyoklion added a commit that referenced this pull request Apr 10, 2024
Adds hook support.

More than 50% of the line count is unit testing and extending contract
test support. I could potentially re-organize to simplify the review if
needed.
kinyoklion added a commit that referenced this pull request Apr 10, 2024
Adds hook support.

More than 50% of the line count is unit testing and extending contract
test support. I could potentially re-organize to simplify the review if
needed.
@github-actions github-actions bot mentioned this pull request Apr 10, 2024
kinyoklion added a commit that referenced this pull request Apr 10, 2024
🤖 I have created a release *beep* *boop*
---


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

##
[1.1.5](akamai-edgeworker-sdk-common-v1.1.4...akamai-edgeworker-sdk-common-v1.1.5)
(2024-04-10)


### Dependencies

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

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

##
[2.1.5](akamai-server-base-sdk-v2.1.4...akamai-server-base-sdk-v2.1.5)
(2024-04-10)


### Dependencies

* The following workspace dependencies were updated
  * dependencies
* @launchdarkly/akamai-edgeworker-sdk-common bumped from ^1.1.4 to
^1.1.5
    * @launchdarkly/js-server-sdk-common bumped from ^2.2.4 to ^2.3.0
</details>

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

##
[1.1.5](akamai-server-edgekv-sdk-v1.1.4...akamai-server-edgekv-sdk-v1.1.5)
(2024-04-10)


### Dependencies

* The following workspace dependencies were updated
  * dependencies
* @launchdarkly/akamai-edgeworker-sdk-common bumped from ^1.1.4 to
^1.1.5
    * @launchdarkly/js-server-sdk-common bumped from ^2.2.4 to ^2.3.0
</details>

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

##
[2.5.2](cloudflare-server-sdk-v2.5.1...cloudflare-server-sdk-v2.5.2)
(2024-04-10)


### Dependencies

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

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

##
[2.3.0](js-server-sdk-common-v2.2.4...js-server-sdk-common-v2.3.0)
(2024-04-10)


### Features

* Implement support for hooks.
([#400](#400))
([14cb044](14cb044))
</details>

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

##
[2.2.5](js-server-sdk-common-edge-v2.2.4...js-server-sdk-common-edge-v2.2.5)
(2024-04-10)


### Dependencies

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

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

##
[9.3.0](node-server-sdk-v9.2.4...node-server-sdk-v9.3.0)
(2024-04-10)

This release introduces a Hooks API. Hooks are collections of
user-defined callbacks that are executed by the SDK at various points of
interest. You can use them to augment the SDK with metrics or tracing.

### Features

* Add support for hooks.
([ce6f041](ce6f041))


### Dependencies

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

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

##
[6.1.8](node-server-sdk-dynamodb-v6.1.7...node-server-sdk-dynamodb-v6.1.8)
(2024-04-10)


### Dependencies

* The following workspace dependencies were updated
  * devDependencies
    * @launchdarkly/node-server-sdk bumped from 9.2.4 to 9.3.0
  * peerDependencies
    * @launchdarkly/node-server-sdk bumped from 9.2.4 to 9.3.0
</details>

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

##
[1.0.0](node-server-sdk-otel-v0.0.1...node-server-sdk-otel-v1.0.0)
(2024-04-10)


### Features

* Add OpenTelemetry support for node-server-sdk.
([#401](#401))
([daf4939](daf4939))


### Dependencies

* The following workspace dependencies were updated
  * devDependencies
    * @launchdarkly/node-server-sdk bumped from 9.2.4 to 9.3.0
  * peerDependencies
    * @launchdarkly/node-server-sdk bumped from 9.2.4 to 9.3.0
</details>

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

##
[4.1.8](node-server-sdk-redis-v4.1.7...node-server-sdk-redis-v4.1.8)
(2024-04-10)


### Dependencies

* The following workspace dependencies were updated
  * devDependencies
    * @launchdarkly/node-server-sdk bumped from 9.2.4 to 9.3.0
  * peerDependencies
    * @launchdarkly/node-server-sdk bumped from 9.2.4 to 9.3.0
</details>

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

##
[1.3.6](vercel-server-sdk-v1.3.5...vercel-server-sdk-v1.3.6)
(2024-04-10)


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * @launchdarkly/js-server-sdk-common-edge bumped from 2.2.4 to 2.2.5
</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>
Co-authored-by: Ryan Lamb <4955475+kinyoklion@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants