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

[NV-3735]: Improve launch darkly #5517

Merged
merged 25 commits into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
53647b3
refactor: Use async launch darkly
antonjoel82 May 6, 2024
4200cd7
refactor: LaunchDarklyProvider
antonjoel82 May 7, 2024
1066caf
refactor: LD Provider with checks
antonjoel82 May 7, 2024
0e7da6a
refactor: Extract LaunchDarklyProvider to shared-web
antonjoel82 May 7, 2024
9389165
fix: Don't show /brand page when IA is enabled
antonjoel82 May 7, 2024
952ec8f
feat: Update loader
antonjoel82 May 7, 2024
e992ffd
fix: Avoid any calls to LD if disabled
antonjoel82 May 7, 2024
a98073d
refactor: Use auth state instead of routes!
antonjoel82 May 7, 2024
961a334
fix: Use config for env var
antonjoel82 May 8, 2024
00bf34e
refactor: Export UserContext
antonjoel82 May 8, 2024
a34a72b
refactor: Use utils for determining LD init
antonjoel82 May 8, 2024
953339e
refactor: Extract util
antonjoel82 May 9, 2024
037d946
refactor: Extract selectors
antonjoel82 May 9, 2024
59b0edb
fix: Hide new layouts route behind flag
antonjoel82 May 9, 2024
c1cc6c9
refactor: Add comment + export
antonjoel82 May 9, 2024
30da614
test: Auth spec
antonjoel82 May 9, 2024
43273d9
test: Wait for feature flags
antonjoel82 May 10, 2024
2ab55ab
refactor: PR Feedback, small comments
antonjoel82 May 12, 2024
6ed5769
refactor: LD Provider in web with associated utils
antonjoel82 May 12, 2024
81ad742
Merge branch 'next' into improve-launch-darkly
antonjoel82 May 12, 2024
1a3ad79
Merge branch 'next' into improve-launch-darkly
antonjoel82 May 13, 2024
9bf9e28
fix: Fix tests, re-order providers
antonjoel82 May 13, 2024
be3c6b9
refactor: Update useAuthController.ts
antonjoel82 May 13, 2024
c511889
fix: Clarify comment
antonjoel82 May 13, 2024
f155b3d
refactor: Move waits
antonjoel82 May 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions apps/web/src/AppRoutes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import { useSettingsRoutes } from './SettingsRoutes';

export const AppRoutes = () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

const isImprovedOnboardingEnabled = useFeatureFlag(FeatureFlagsKeysEnum.IS_IMPROVED_ONBOARDING_ENABLED);
const isInformationArchitectureEnabled = useFeatureFlag(FeatureFlagsKeysEnum.IS_INFORMATION_ARCHITECTURE_ENABLED);

return (
<Routes>
Expand Down Expand Up @@ -116,13 +117,16 @@ export const AppRoutes = () => {
<Route path={ROUTES.TEAM} element={<MembersInvitePage />} />
<Route path={ROUTES.CHANGES} element={<PromoteChangesPage />} />
<Route path={ROUTES.SUBSCRIBERS} element={<SubscribersList />} />
<Route path={ROUTES.BRAND} element={<BrandPage />}>
<Route path="" element={<BrandingForm />} />
<Route path="layouts" element={<LayoutsListPage />} />
</Route>
<Route path={ROUTES.LAYOUT} element={<LayoutsPage />}>
<Route path="" element={<LayoutsListPage />} />
</Route>
{!isInformationArchitectureEnabled ? (
<Route path={ROUTES.BRAND} element={<BrandPage />}>
<Route path="" element={<BrandingForm />} />
<Route path="layouts" element={<LayoutsListPage />} />
</Route>
) : (
<Route path={ROUTES.LAYOUT} element={<LayoutsPage />}>
<Route path="" element={<LayoutsListPage />} />
</Route>
)}
<Route path="/translations/*" element={<TranslationRoutes />} />
</Route>
</Routes>
Expand Down
49 changes: 32 additions & 17 deletions apps/web/src/Providers.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import { CONTEXT_PATH, LAUNCH_DARKLY_CLIENT_SIDE_ID, SegmentProvider } from '@novu/shared-web';
import { Loader } from '@mantine/core';
import { colors } from '@novu/design-system';
import { CONTEXT_PATH, LaunchDarklyProvider, SegmentProvider } from '@novu/shared-web';
import * as Sentry from '@sentry/react';
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
import { withLDProvider } from 'launchdarkly-react-client-sdk';
import { PropsWithChildren } from 'react';
import { HelmetProvider } from 'react-helmet-async';
import { BrowserRouter } from 'react-router-dom';
import { api } from './api/api.client';
import { AuthProvider } from './components/providers/AuthProvider';
import { css } from './styled-system/css';

const defaultQueryFn = async ({ queryKey }: { queryKey: string }) => {
const response = await api.get(`${queryKey[0]}`);
Expand All @@ -22,28 +24,41 @@ const queryClient = new QueryClient({
},
});

/** Full-page loader that uses color-preferences for background */
const fallbackDisplay = (
<div
className={css({
h: '100dvh',
w: '100dvw',
display: 'grid',
placeItems: 'center',
bg: 'surface.page',
// Root element may not have loaded so rely on OS
_osDark: { bg: 'legacy.BGDark' },
_osLight: { bg: 'legacy.BGLight' },
})}
>
<Loader size={64} variant="bars" color={colors.gradientMiddle} />
</div>
);

/**
* Centralized Provider hierarchy.
*/
const Providers: React.FC<PropsWithChildren<{}>> = ({ children }) => {
return (
<SegmentProvider>
<HelmetProvider>
<BrowserRouter basename={CONTEXT_PATH}>
<QueryClientProvider client={queryClient}>
<AuthProvider>{children}</AuthProvider>
</QueryClientProvider>
</BrowserRouter>
</HelmetProvider>
<QueryClientProvider client={queryClient}>
<AuthProvider>
<LaunchDarklyProvider fallbackDisplay={fallbackDisplay}>
<HelmetProvider>
<BrowserRouter basename={CONTEXT_PATH}>{children}</BrowserRouter>
</HelmetProvider>
</LaunchDarklyProvider>
</AuthProvider>
</QueryClientProvider>
</SegmentProvider>
);
};

export default Sentry.withProfiler(
withLDProvider({
clientSideID: LAUNCH_DARKLY_CLIENT_SIDE_ID,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

question: why move to shared-web?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

🤓 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, thanks for the feedback! Updated

reactOptions: {
useCamelCaseFlagKeys: false,
},
})(Providers)
);
export default Sentry.withProfiler(Providers);
2 changes: 0 additions & 2 deletions apps/web/src/SettingsRoutes.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import { ProductLead } from './components/utils/ProductLead';
import { ROUTES } from './constants/routes.enum';
import { useFeatureFlag } from './hooks';
import { BillingRoutes } from './pages/BillingPages';
import { BrandingForm as BrandingFormOld } from './pages/brand/tabs';
import { BrandingPage } from './pages/brand/tabs/v2';
import { MembersInvitePage as MembersInvitePageNew } from './pages/invites/v2/MembersInvitePage';
import { AccessSecurityPage, ApiKeysPage, BillingPage, TeamPage, UserProfilePage } from './pages/settings';
Expand Down Expand Up @@ -50,7 +49,6 @@ export const useSettingsRoutes = () => {
<Route path="billing/*" element={<BillingRoutes />} />
<Route path="email" element={<EmailSettings />} />
<Route path="team" element={<MembersInvitePageNew />} />
<Route path="brand" element={<BrandingFormOld />} />
<Route
path="permissions"
element={
Expand Down
5 changes: 2 additions & 3 deletions libs/shared-web/src/hooks/useAuthController.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { useEffect, useCallback, useState } from 'react';
import axios from 'axios';
import jwtDecode from 'jwt-decode';
import { useNavigate } from 'react-router-dom';
import { useQuery, useQueryClient } from '@tanstack/react-query';
import * as Sentry from '@sentry/react';
import type { IJwtPayload, IOrganizationEntity, IUserEntity } from '@novu/shared';
Expand Down Expand Up @@ -41,7 +40,6 @@ export function getToken(): string {
export function useAuthController() {
const segment = useSegment();
const queryClient = useQueryClient();
const navigate = useNavigate();
const [token, setToken] = useState<string | null>(() => {
const initialToken = getToken();
applyToken(initialToken);
Expand Down Expand Up @@ -122,7 +120,8 @@ export function useAuthController() {
const logout = () => {
setTokenCallback(null);
queryClient.clear();
navigate('/auth/login');
// avoid usage of react-router here to prevent needing AuthProvider to be wrapped in the BrowserRouter
window.location.assign('/auth/login');
antonjoel82 marked this conversation as resolved.
Show resolved Hide resolved
segment.reset();
};

Expand Down
16 changes: 9 additions & 7 deletions libs/shared-web/src/hooks/useFeatureFlags.ts
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
import { FeatureFlagsKeysEnum, IOrganizationEntity, prepareBooleanStringFeatureFlag } from '@novu/shared';
import { useFlags } from 'launchdarkly-react-client-sdk';
import { useLDClient } from 'launchdarkly-react-client-sdk';
import { useFlags, useLDClient } from 'launchdarkly-react-client-sdk';
import { useEffect } from 'react';
import { checkShouldUseLaunchDarkly } from '../utils';

import { FEATURE_FLAGS } from '../config';

export const useFeatureFlags = (organization: IOrganizationEntity) => {
export const useFeatureFlags = (organization?: IOrganizationEntity) => {
const ldClient = useLDClient();

useEffect(() => {
if (!organization?._id) {
if (!checkShouldUseLaunchDarkly() || !organization?._id || !ldClient) {
return;
}

ldClient?.identify({
ldClient.identify({
kind: 'organization',
key: organization._id,
name: organization.name,
Expand All @@ -24,10 +24,12 @@ export const useFeatureFlags = (organization: IOrganizationEntity) => {
};

export const useFeatureFlag = (key: FeatureFlagsKeysEnum): boolean => {
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;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

const fallbackValue = false;
const value = FEATURE_FLAGS[key];
const defaultValue = prepareBooleanStringFeatureFlag(value, fallbackValue);

return featureFlag ?? defaultValue;
return flagValue ?? defaultValue;
};
1 change: 1 addition & 0 deletions libs/shared-web/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ export * from './hooks';
export * from './providers';
export * from './constants';
export * from './components';
export * from './utils';
11 changes: 7 additions & 4 deletions libs/shared-web/src/providers/AuthProvider.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import React, { useContext } from 'react';
import { IOrganizationEntity, IUserEntity, IJwtPayload } from '@novu/shared';
import { useAuthController, useFeatureFlags } from '../hooks';
import { useAuthController } from '../hooks';

type UserContext = {
export type UserContext = {
token: string | null;
isLoggedIn: boolean;
currentUser: IUserEntity | undefined;
isUserLoading: boolean;
currentOrganization: IOrganizationEntity | undefined;
Expand All @@ -15,6 +16,7 @@ type UserContext = {

const AuthContext = React.createContext<UserContext>({
token: null,
isLoggedIn: false,
currentUser: undefined,
isUserLoading: true,
setToken: undefined as any,
Expand All @@ -27,13 +29,14 @@ const AuthContext = React.createContext<UserContext>({
export const useAuthContext = (): UserContext => useContext(AuthContext);

export const AuthProvider = ({ children }: { children: React.ReactNode }) => {
const { token, setToken, user, organization, isUserLoading, logout, jwtPayload, organizations } = useAuthController();
useFeatureFlags(organization);
const { token, setToken, user, organization, isUserLoading, logout, jwtPayload, organizations, isLoggedIn } =
useAuthController();

return (
<AuthContext.Provider
value={{
currentUser: user,
isLoggedIn,
isUserLoading,
currentOrganization: organization,
organizations,
Expand Down
100 changes: 100 additions & 0 deletions libs/shared-web/src/providers/LaunchDarklyProvider.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import { IOrganizationEntity } from '@novu/shared';
import { asyncWithLDProvider } from 'launchdarkly-react-client-sdk';
import { PropsWithChildren, ReactNode, useEffect, useMemo, useRef, useState } from 'react';
import { LAUNCH_DARKLY_CLIENT_SIDE_ID } from '../config';
import { useFeatureFlags } from '../hooks';
import { useAuthContext } from './AuthProvider';
import { selectShouldShowLaunchDarklyFallback, selectShouldInitializeLaunchDarkly } from '../utils/auth-selectors';

/** A provider with children required */
type GenericLDProvider = Awaited<ReturnType<typeof asyncWithLDProvider>>;

/** Simply renders the children */
const DEFAULT_GENERIC_PROVIDER: GenericLDProvider = ({ children }) => <>{children}</>;

export interface ILaunchDarklyProviderProps {
/** Renders when LaunchDarkly is enabled and is awaiting initialization */
fallbackDisplay: ReactNode;
}

/**
* Async provider for feature flags.
*
* @requires AuthProvider must be wrapped in the AuthProvider.
*/
export const LaunchDarklyProvider: React.FC<PropsWithChildren<ILaunchDarklyProviderProps>> = ({
children,
fallbackDisplay,
}) => {
const LDProvider = useRef<GenericLDProvider>(DEFAULT_GENERIC_PROVIDER);
const [isLDReady, setIsLDReady] = useState<boolean>(false);
antonjoel82 marked this conversation as resolved.
Show resolved Hide resolved

const authContext = useAuthContext();
if (!authContext) {
throw new Error('LaunchDarklyProvider must be used within <AuthProvider>!');
}
const { currentOrganization } = authContext;

const shouldInitializeLd = useMemo(() => selectShouldInitializeLaunchDarkly(authContext), [authContext]);

useEffect(() => {
if (!shouldInitializeLd) {
return;
}

const fetchLDProvider = async () => {
try {
LDProvider.current = await asyncWithLDProvider({
clientSideID: LAUNCH_DARKLY_CLIENT_SIDE_ID,
reactOptions: {
useCamelCaseFlagKeys: false,
},
// determine which context to use based on if an organization is available
context: currentOrganization
? {
kind: 'organization',
key: currentOrganization._id,
name: currentOrganization.name,
}
: {
/**
* When user is not authenticated, assigns an id to them to ensure consistent results.
* https://docs.launchdarkly.com/sdk/features/anonymous#javascript
*/
kind: 'user',
anonymous: true,
},
});
} catch (err: unknown) {
// FIXME: what should we do here since we don't have logging?
antonjoel82 marked this conversation as resolved.
Show resolved Hide resolved
} finally {
setIsLDReady(true);
}
};

fetchLDProvider();
}, [setIsLDReady, shouldInitializeLd, currentOrganization]);

/**
* For self-hosted, LD will not be enabled, so do not block initialization.
* Must not show the fallback if the user isn't logged-in to avoid issues with un-authenticated routes (i.e. login).
*/
if (selectShouldShowLaunchDarklyFallback(authContext, isLDReady)) {
return <>{fallbackDisplay}</>;
}

return (
<LDProvider.current>
<LaunchDarklyClientWrapper org={currentOrganization}>{children}</LaunchDarklyClientWrapper>
</LDProvider.current>
);
};

/**
* Refreshes feature flags on org change using the LaunchDarkly client from the provider.
*/
function LaunchDarklyClientWrapper({ children, org }: PropsWithChildren<{ org?: IOrganizationEntity }>) {
useFeatureFlags(org);

return <>{children}</>;
}
antonjoel82 marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions libs/shared-web/src/providers/index.ts
Original file line number Diff line number Diff line change
@@ -1,2 +1,3 @@
export * from './SegmentProvider';
export * from './AuthProvider';
export * from './LaunchDarklyProvider';
3 changes: 3 additions & 0 deletions libs/shared-web/src/utils/auth-selectors/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export * from './selectHasUserCompletedSignUp';
export * from './selectShouldShowLaunchDarklyFallback';
export * from './selectShouldInitializeLaunchDarkly';
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { UserContext } from '../../providers';

/**
* Determine if a user is fully-registered; if not, they're still in onboarding.
*/
export const selectHasUserCompletedSignUp = (userCtx: UserContext): boolean => {
if (!userCtx) {
return false;
}

// User has completed registration if they have an associated orgId.
return !!userCtx.jwtPayload?.organizationId;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

🗒 note (non-blocking): This does not seem like the best way, but it is the most robust and semantic way I found thus far‏

};
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 wanted to add tests to these files, but we do not have testing set up yet in shared-web 😞

https://linear.app/novu/issue/NV-3773/setup-tests-in-shared-web-and-implement-tests-for-launchdarkly

Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { selectHasUserCompletedSignUp } from './selectHasUserCompletedSignUp';
import { checkShouldUseLaunchDarkly } from '../checkShouldUseLaunchDarkly';
import { UserContext } from '../../providers/AuthProvider';

/** Determine if LaunchDarkly should be initialized based on the current auth context */
export function selectShouldInitializeLaunchDarkly(userCtx: UserContext): boolean {
const { isLoggedIn, currentOrganization } = userCtx;
// don't show fallback if LaunchDarkly isn't enabled
if (!checkShouldUseLaunchDarkly()) {
return false;
}

// enable feature flags for unauthenticated areas of the app
if (!isLoggedIn) {
antonjoel82 marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

// allow LD to load when the user is created but still in onboarding
if (!selectHasUserCompletedSignUp(userCtx)) {
antonjoel82 marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

// if a user is fully on-boarded, but no organization has loaded, we must wait for the organization to initialize the client.
return !!currentOrganization;
}
Loading
Loading