[NV-3735]: Improve launch darkly #5517

antonjoel82 · 2024-05-07T17:47:44Z

What changed? Why was the change needed?

Use async LaunchDarkly initialization, but ensure it works with self-hosted
Why? Conditional routing (enabling routes based on feature flags) for Information Architecture is inconsistent, and causes issues with certain routes
Resolves NV-3735

Special notes for your reviewer

Did we account properly for self-hosted?

Repro

Basic

Check-out the branch improve-launch-darkly
Start everything normally -- upon loading, you should briefly see a reddish loader in the center of the page
Open a new tab and navigate to http://localhost:4200/settings/profile?view=password
You should see the loader again, but will go directly the the User Profile with the sidebar open

Old route hidden behind feature flag

Navigate to http://localhost:4200/brand
The loader should appear, and you should be directed to Workflows

Check non-auth route

Log-out
Everything should load correctly.
Log-in
The loader should appear and the app should then load

Self-hosted

In your local web/.env.local file (or whichever one you're using for local dev), comment out REACT_APP_LAUNCH_DARKLY_CLIENT_SIDE_ID
Start the app
Open the browser tools to the Network tab
The app should load correctly without the loader, but everything will be based on env variables
Filter network requests with launchd and nothing should appear

linear · 2024-05-07T17:47:47Z

NV-3735 Refactor LaunchDarkly Implementation to Better Support Conditional Routing

netlify · 2024-05-07T17:48:01Z

✅ Deploy Preview for novu-design ready!

Name	Link
🔨 Latest commit	`80ee88d`
🔍 Latest deploy log	https://app.netlify.com/sites/novu-design/deploys/663a69422242c9000879c7a9
😎 Deploy Preview	https://deploy-preview-5517--novu-design.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2024-05-07T17:48:53Z

✅ Deploy Preview for dev-web-novu ready!

Name	Link
🔨 Latest commit	`f155b3d`
🔍 Latest deploy log	https://app.netlify.com/sites/dev-web-novu/deploys/6642b3e49efd0f000824f047
😎 Deploy Preview	https://deploy-preview-5517--dev-web-novu.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2024-05-07T17:52:32Z

✅ Deploy Preview for novu-design ready!

Name	Link
🔨 Latest commit	`f155b3d`
🔍 Latest deploy log	https://app.netlify.com/sites/novu-design/deploys/6642b3e412f45b0008afc1d3
😎 Deploy Preview	https://deploy-preview-5517--novu-design.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

antonjoel82 · 2024-05-07T18:12:59Z

apps/web/src/AppRoutes.tsx

-          <Route path="" element={<BrandingForm />} />
-          <Route path="layouts" element={<LayoutsListPage />} />
-        </Route>
+        {!isInformationArchitectureEnabled && (


🗒 note (non-blocking): Fix for information architecture -- previously didn't work due to LD‏

antonjoel82 · 2024-05-07T18:13:55Z

apps/web/src/Providers.tsx

    </SegmentProvider>
  );
 };

-export default Sentry.withProfiler(
-  withLDProvider({


🗒 note (non-blocking): Moved LD provider to its own file in shared-web with logic‏

❓ question: why move to shared-web?

Most of our other similar providers are there, and many of the related behaviors / imports (i.e. useFeatureFlags) are there, so it seemed best IMO

🤓 nitpick (non-blocking): I suggest keeping it closer to the source of the web app unless we are sure it would be reused. Otherwise, we might end up with a landfill.

I agree with @LetItRock. I feel that we might be overusing shared packages on some occasions. Other than that, great choice to split it into a separate component.

Sounds good, thanks for the feedback! Updated

libs/shared-web/src/constants/unprotected-routes.const.ts

antonjoel82 · 2024-05-07T18:49:00Z

libs/shared-web/src/hooks/useFeatureFlags.ts

-  const { [key]: featureFlag } = useFlags();
+  /** We knowingly break the rule of hooks here to avoid making any LaunchDarkly calls when it is disabled */
+  // eslint-disable-next-line
+  const flagValue = checkShouldUseLaunchDarkly() ? useFlags()[key] : undefined;


🗒 note (non-blocking): Breaks the rule of hooks, but should be okay since it's done via a static check.‏

antonjoel82 · 2024-05-07T18:50:00Z

libs/shared-web/src/hooks/useFeatureFlags.ts

  const ldClient = useLDClient();

  useEffect(() => {
-    if (!organization?._id) {
+    if (!checkShouldUseLaunchDarkly() || !organization?._id) {


🗒 note (non-blocking): The client should never be defined anyway, but just as an extra fail-safe‏

antonjoel82 · 2024-05-07T18:52:52Z

libs/shared-web/src/providers/LaunchDarklyProvider.tsx

+
+  useEffect(() => {
+    // no need to fetch if LD is disabled or there isn't an org to query against
+    if (!checkShouldUseLaunchDarkly() || !currentOrganization) {


🗒 note (non-blocking): Avoids any LD calls for initializing the provider‏

scopsy · 2024-05-07T21:45:18Z

libs/shared-web/src/providers/LaunchDarklyProvider.tsx

+          kind: 'organization',
+          key: currentOrganization._id,
+          name: currentOrganization.name,


question: If I understand this correctly, this will prevent us from having feature flags on the Onboarding and Signup pages right?

What if we still load launchdarkly in those pages but without the "context" ?

‏

I'll investigate further. It definitely introduces some complexity, since they require fundamentally different things. Additionally, we're not handling this properly at the moment

LetItRock

⚠ issue: blockers: sign up process and reset password are not working for me

Screen.Recording.2024-05-08.at.00.29.23.mov

Please make sure to run all e2e tests with this PR.

libs/shared-web/src/hooks/useAuthController.ts

LetItRock · 2024-05-07T21:40:26Z

apps/web/src/Providers.tsx

    </SegmentProvider>
  );
 };

-export default Sentry.withProfiler(
-  withLDProvider({


❓ question: why move to shared-web?

libs/shared-web/src/utils/checkShouldUseLaunchDarkly.ts

libs/shared-web/src/providers/LaunchDarklyProvider.tsx

LetItRock · 2024-05-07T22:28:46Z

apps/web/src/AppRoutes.tsx

@@ -49,6 +49,7 @@ import { useSettingsRoutes } from './SettingsRoutes';

 export const AppRoutes = () => {


🗒 note (non-blocking): the /auth/application is not a protected route 😨

antonjoel82 · 2024-05-08T16:34:56Z

⚠ issue: blockers: sign up process and reset password are not working for me

Screen.Recording.2024-05-08.at.00.29.23.mov
Please make sure to run all e2e tests with this PR.

Great catch, thanks! This whole non-automated automated tests is beating me 🙃

SokratisVidros · 2024-05-10T10:27:11Z

apps/web/src/Providers.tsx

    </SegmentProvider>
  );
 };

-export default Sentry.withProfiler(
-  withLDProvider({


I agree with @LetItRock. I feel that we might be overusing shared packages on some occasions. Other than that, great choice to split it into a separate component.

libs/shared-web/src/hooks/useAuthController.ts

libs/shared-web/src/utils/auth-selectors/selectShouldInitializeLaunchDarkly.tsx

antonjoel82 · 2024-05-14T00:46:48Z

Unfortunately, I am encountering some flakiness with tests, but I have verified manually that the behavior works. After talking with Sok this morning, we don't believe it's worth blocking on them since we'll be replacing them soon. Planning to merge this tomorrow morning after deployment breakfast

Use async LaunchDarkly initialization, but ensure it works with self-hosted Why? Conditional routing (enabling routes based on feature flags) for Information Architecture is inconsistent, and causes issues with certain routes Resolves NV-3735

github-actions bot added the @novu/web label May 7, 2024

antonjoel82 commented May 7, 2024

View reviewed changes

libs/shared-web/src/constants/unprotected-routes.const.ts Outdated Show resolved Hide resolved

antonjoel82 marked this pull request as ready for review May 7, 2024 18:47

antonjoel82 requested a review from a team May 7, 2024 18:47

antonjoel82 commented May 7, 2024

View reviewed changes

antonjoel82 requested a review from scopsy May 7, 2024 19:47

scopsy reviewed May 7, 2024

View reviewed changes

LetItRock reviewed May 7, 2024

View reviewed changes

antonjoel82 marked this pull request as draft May 8, 2024 22:17

antonjoel82 force-pushed the improve-launch-darkly branch from c68d35c to c51c649 Compare May 9, 2024 05:53

github-actions bot added @novu/api @novu/dal @novu/notification-center @novu/shared @novu/worker CI/CD @novu/design-system labels May 9, 2024

antonjoel82 added 4 commits May 8, 2024 22:54

refactor: Use async launch darkly

53647b3

refactor: LaunchDarklyProvider

4200cd7

refactor: LD Provider with checks

1066caf

refactor: Extract LaunchDarklyProvider to shared-web

0e7da6a

antonjoel82 temporarily deployed to Development May 9, 2024 17:50 — with GitHub Actions Inactive

antonjoel82 temporarily deployed to Development May 10, 2024 16:34 — with GitHub Actions Inactive

antonjoel82 had a problem deploying to Development May 10, 2024 16:34 — with GitHub Actions Failure

antonjoel82 temporarily deployed to Development May 10, 2024 17:07 — with GitHub Actions Inactive

test: Wait for feature flags

43273d9

antonjoel82 temporarily deployed to Development May 10, 2024 21:38 — with GitHub Actions Inactive

antonjoel82 had a problem deploying to Development May 10, 2024 21:38 — with GitHub Actions Failure

antonjoel82 temporarily deployed to Development May 10, 2024 21:38 — with GitHub Actions Inactive

antonjoel82 had a problem deploying to Development May 10, 2024 21:38 — with GitHub Actions Failure

antonjoel82 temporarily deployed to Development May 10, 2024 21:38 — with GitHub Actions Inactive

antonjoel82 temporarily deployed to Development May 10, 2024 21:56 — with GitHub Actions Inactive

SokratisVidros reviewed May 10, 2024

View reviewed changes

antonjoel82 added 3 commits May 12, 2024 10:14

refactor: PR Feedback, small comments

2ab55ab

refactor: LD Provider in web with associated utils

6ed5769

Merge branch 'next' into improve-launch-darkly

81ad742

SokratisVidros approved these changes May 13, 2024

View reviewed changes

antonjoel82 added 5 commits May 13, 2024 09:51

Merge branch 'next' into improve-launch-darkly

1a3ad79

fix: Fix tests, re-order providers

9bf9e28

refactor: Update useAuthController.ts

be3c6b9

fix: Clarify comment

c511889

refactor: Move waits

f155b3d

antonjoel82 merged commit 49600bf into next May 14, 2024
34 of 36 checks passed

antonjoel82 deleted the improve-launch-darkly branch May 14, 2024 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NV-3735]: Improve launch darkly #5517

[NV-3735]: Improve launch darkly #5517

antonjoel82 commented May 7, 2024 •

edited

Loading

linear bot commented May 7, 2024

netlify bot commented May 7, 2024 •

edited

Loading

netlify bot commented May 7, 2024 •

edited

Loading

netlify bot commented May 7, 2024 •

edited

Loading

antonjoel82 May 7, 2024

antonjoel82 May 7, 2024

LetItRock May 7, 2024

antonjoel82 May 9, 2024

LetItRock May 9, 2024

SokratisVidros May 10, 2024

antonjoel82 May 12, 2024

antonjoel82 May 7, 2024

antonjoel82 May 7, 2024

antonjoel82 May 7, 2024

scopsy May 7, 2024

antonjoel82 May 8, 2024

LetItRock left a comment

LetItRock May 7, 2024

LetItRock May 7, 2024

antonjoel82 commented May 8, 2024

SokratisVidros May 10, 2024

antonjoel82 commented May 14, 2024

		@@ -49,6 +49,7 @@ import { useSettingsRoutes } from './SettingsRoutes';

		export const AppRoutes = () => {

[NV-3735]: Improve launch darkly #5517

[NV-3735]: Improve launch darkly #5517

Conversation

antonjoel82 commented May 7, 2024 • edited Loading

What changed? Why was the change needed?

Special notes for your reviewer

Repro

linear bot commented May 7, 2024

netlify bot commented May 7, 2024 • edited Loading

✅ Deploy Preview for novu-design ready!

netlify bot commented May 7, 2024 • edited Loading

✅ Deploy Preview for dev-web-novu ready!

netlify bot commented May 7, 2024 • edited Loading

✅ Deploy Preview for novu-design ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LetItRock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonjoel82 commented May 8, 2024

Choose a reason for hiding this comment

antonjoel82 commented May 14, 2024

antonjoel82 commented May 7, 2024 •

edited

Loading

netlify bot commented May 7, 2024 •

edited

Loading

netlify bot commented May 7, 2024 •

edited

Loading

netlify bot commented May 7, 2024 •

edited

Loading