fix: Increment version for changing flagValues #317
Conversation
|
This pull request has been linked to Shortcut Story #224098: FileDataSource doesn't emit updates when just using flagValues.. |
kinyoklion
requested review from
yusinto and
louis-launchdarkly
as code owners
|
@fhawkes If you are setup for development could you give this a try and see if it solves your problems? |
|
There are some additional mismatches in typing that should be addressed. In specifically the FileDataSourceFactory. The old implementation had access to the config directly and was able to access the featureStore. This typing remained, which means it isn't completely correct. We don't have an external type for an update only feature store abstraction, but we 100% should. This would be a breaking change, so I am not incorporating it as part of this change. Generally speaking it isn't an interface people implement often unless they are attempting to implement a custom update processor, which is a leaky abstraction we have maintained for backward compatibility. |
| // If the data is different, then we want to increment the version. | ||
| if ( | ||
| previousInstance && | ||
| JSON.stringify(parsed.flagValues[key]) !== JSON.stringify(previousInstance?.variations?.[0]) |
There was a problem hiding this comment.
This isn't deterministic (it usually will be, but sometimes the JSON properties may serialize in different orders). So we may want to do something different here.
There was a problem hiding this comment.
We don't recommend using FileDataSource in production, so this may be sufficient.
There was a problem hiding this comment.
The different ordering is a concern, and the best way to deep compare is to use lodash's isEqual function. I'm reluctant to introduce dependency on lodash/is-equal, so if FileDataSource is only used in dev, I think this is ok for now.
There was a problem hiding this comment.
I think we can do with this, and then change when we can. Most flags aren't structured as well.
| return { | ||
| key, | ||
| on: true, | ||
| fallthrough: { variation: 0 }, | ||
| variations: [value], | ||
| version: 1, | ||
| version: version ?? 1, |
There was a problem hiding this comment.
Is null coalescing needed here? version is a number and should not be null or undefined.
| version: version ?? 1, | |
| version, |
There was a problem hiding this comment.
Interesting that linting doesn't catch this. It was leftover from an interim version. Thank you for noticing it.
Hey @kinyoklion I was able to test these changes out and they do solve the issue for us! I'm now seeing flag changes get emitted as expected. Thanks!! |
|
Any movement on this one? |
|
Hey @fhawkes, No movement. I will see if I can get to it soon. Thank you, |
🤖 I have created a release *beep* *boop*
---
<details><summary>@launchdarkly/akamai-edgeworker-sdk-common:
1.0.4</summary>
### Dependencies
* The following workspace dependencies were updated
* dependencies
* @launchdarkly/js-server-sdk-common bumped from ^2.1.0 to ^2.1.1
</details>
<details><summary>@launchdarkly/akamai-server-base-sdk: 2.0.4</summary>
### Dependencies
* The following workspace dependencies were updated
* dependencies
* @launchdarkly/akamai-edgeworker-sdk-common bumped from ^1.0.3 to
^1.0.4
* @launchdarkly/js-server-sdk-common bumped from ^2.1.0 to ^2.1.1
</details>
<details><summary>@launchdarkly/akamai-server-edgekv-sdk:
1.0.12</summary>
### Dependencies
* The following workspace dependencies were updated
* dependencies
* @launchdarkly/akamai-edgeworker-sdk-common bumped from ^1.0.3 to
^1.0.4
* @launchdarkly/js-server-sdk-common bumped from ^2.1.0 to ^2.1.1
</details>
<details><summary>@launchdarkly/cloudflare-server-sdk: 2.3.1</summary>
### Dependencies
* The following workspace dependencies were updated
* devDependencies
* @launchdarkly/js-server-sdk-common-edge bumped from 2.1.0 to 2.1.1
</details>
<details><summary>js-server-sdk-common: 2.1.1</summary>
##
[2.1.1](js-server-sdk-common-v2.1.0...js-server-sdk-common-v2.1.1)
(2023-12-04)
### Bug Fixes
* Increment version for changing flagValues
([#317](#317))
([e8e07ef](e8e07ef))
</details>
<details><summary>@launchdarkly/js-server-sdk-common-edge:
2.1.1</summary>
### Dependencies
* The following workspace dependencies were updated
* dependencies
* @launchdarkly/js-server-sdk-common bumped from 2.1.0 to 2.1.1
</details>
<details><summary>@launchdarkly/node-server-sdk: 9.0.4</summary>
### Dependencies
* The following workspace dependencies were updated
* dependencies
* @launchdarkly/js-server-sdk-common bumped from 2.1.0 to 2.1.1
</details>
<details><summary>@launchdarkly/node-server-sdk-dynamodb:
6.0.4</summary>
### Dependencies
* The following workspace dependencies were updated
* devDependencies
* @launchdarkly/node-server-sdk bumped from 9.0.3 to 9.0.4
</details>
<details><summary>@launchdarkly/node-server-sdk-redis: 4.0.4</summary>
### Dependencies
* The following workspace dependencies were updated
* devDependencies
* @launchdarkly/node-server-sdk bumped from 9.0.3 to 9.0.4
</details>
<details><summary>@launchdarkly/vercel-server-sdk: 1.2.1</summary>
### Dependencies
* The following workspace dependencies were updated
* dependencies
* @launchdarkly/js-server-sdk-common-edge bumped from 2.1.0 to 2.1.1
</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 <[email protected]>
When using FileData you have the option of just providing values. The pre-typescript SDK was liberal with what it considered an update and it would emit updates as things changed.
Some improvements to prevent spurious update events were added in typscript and these interfered with the basic way the FileData implementation works.
This PR makes a couple updates:
1.) If the value of a "flagValue" changes, then we will increment the version associated with it.
2.) If the value of a "flagValue" does not change, then we will not increment the version.
This should mean when values change the version will change. Standard flag/segment JSON must update their versions manually.
This will add some performance overhead.
This addresses #310