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.

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

refactor: [M3-7750] - Update launchdarkly-react-client-sdk to latest #10165

Conversation

bnussman-akamai
Copy link
Contributor

Description πŸ“

Updates Launch Darkly to latest πŸ“¦

Changes πŸ”„

  • Updates Launch Darkly to latest πŸ“¦ and addresses any breaking changes
  • Refactors withFeatureFlagConsumer ➑️ withFeatureFlags because it needed to be cleaned up 🧼

How to test πŸ§ͺ

Prerequisites

  • Check out this PR
  • Run yarn in the root directory to make sure updated dependencies are installed

Verification steps

  • Verify feature flags work as expected
  • Verify unit tests handle feature flags properly
  • Verify my refactor of withFeatureFlagConsumer ➑️ withFeatureFlags does not introduce any breaking changes

As an Author I have considered πŸ€”

  • πŸ‘€ Doing a self review
  • ❔ Our contribution guidelines
  • 🀏 Splitting feature into small PRs
  • βž• Adding a changeset
  • πŸ§ͺ Providing/Improving test coverage
  • πŸ” Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • πŸ‘£ Providing comprehensive reproduction steps
  • πŸ“‘ Providing or updating our documentation
  • πŸ•› Scheduling a pair reviewing session
  • πŸ“± Providing mobile support
  • β™Ώ Providing accessibility support

@bnussman-akamai bnussman-akamai added the Dependencies Pull requests that update a dependency file label Feb 8, 2024
@bnussman-akamai bnussman-akamai self-assigned this Feb 8, 2024
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner February 8, 2024 17:07
@bnussman-akamai bnussman-akamai requested review from carrillo-erik and hkhalil-akamai and removed request for a team February 8, 2024 17:07
@@ -23,37 +22,35 @@ import { useSetupFeatureFlags } from './useSetupFeatureFlags';
export const App = () => <BaseApp />;

const BaseApp = withDocumentTitleProvider(
withFeatureFlagProvider(
withFeatureFlagConsumer(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The consumer is not needed here because we don't need to read flags in this component.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just a much cleaner rewrite of withFeatureFlagConsumer.container.ts

DomainRecord[],
DomainRecord[],
DomainRecord[]
>(prependLinodeNS, filter(typeEq('NS')), pathOr([], ['domainRecords']));

export default recompose<CombinedProps, DomainRecordsProps>(withFeatureFlags)(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This component does not consume feature flags

@@ -432,7 +431,6 @@ const sendGroupByAnalytic = (value: boolean) => {

export const enhanced = compose<CombinedProps, LinodesLandingProps>(
withRouter,
withFeatureFlagConsumer,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This component does not consume feature flags so this can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had to address a breaking change here. Launch Darkly no longer exports Provider so we have to use LDProvider.

See launchdarkly/react-client-sdk#30 (comment)

@@ -39,7 +39,7 @@
"ipaddr.js": "^1.9.1",
"jspdf": "^2.3.1",
"jspdf-autotable": "^3.5.14",
"launchdarkly-react-client-sdk": "^3.0.6",
"launchdarkly-react-client-sdk": "3.0.10",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm going to start pinning exact versions (without the ^) so that if we ever have to re-generate the lockfile, the minor version won't update. Usually this is fine because minor versions shouldn't case breaking changes, but in this case for launch darkly, it did.

Using the exact version will make things more predictable and safe

Copy link

github-actions bot commented Feb 8, 2024

Coverage Report: βœ…
Base Coverage: 81.12%
Current Coverage: 81.12%

Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Tested feature flags and affected components. Everything looks good, thanks for the update and clean up work @bnussman-akamai!

Minor style suggestions from eslint

@bnussman-akamai bnussman-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Feb 9, 2024
clientSideID={''}
deferInitialization
flags={options.flags ?? {}}
options={{ bootstrap: options.flags }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, here is some context for others:

The bootstrapping feature lets you decrease startup times for client-side SDKs by providing them with an initial set of flag values that are immediately available during client initialization.

@jaalah-akamai jaalah-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Feb 9, 2024
@bnussman-akamai bnussman-akamai merged commit 586a24e into linode:develop Feb 9, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants