-
Notifications
You must be signed in to change notification settings - Fork 235
feat: Compass Experimentation Provider CLOUDP-333843 #7151
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
Changes from 5 commits
d1ff4f7
8a3704e
34155ed
87680b7
19b0953
533719e
4614599
7c72f4b
9f9b0ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,60 @@ | ||||||||||||||||
import React, { createContext, useContext, useRef } from 'react'; | ||||||||||||||||
import type { types } from '@mongodb-js/mdb-experiment-js'; | ||||||||||||||||
import type { typesReact } from '@mongodb-js/mdb-experiment-js/react'; | ||||||||||||||||
|
||||||||||||||||
type UseAssignmentHookFn = ( | ||||||||||||||||
experimentName: string, | ||||||||||||||||
trackIsInSample: boolean, | ||||||||||||||||
options?: types.GetAssignmentOptions<types.TypeData> | ||||||||||||||||
) => typesReact.UseAssignmentResponse<types.TypeData>; | ||||||||||||||||
|
||||||||||||||||
type AssignExperimentFn = ( | ||||||||||||||||
experimentName: string, | ||||||||||||||||
options?: types.AssignOptions<string> | ||||||||||||||||
) => Promise<'SUCCESS' | 'ERROR' | null>; | ||||||||||||||||
|
||||||||||||||||
interface CompassExperimentationProviderContextValue { | ||||||||||||||||
useAssignment: UseAssignmentHookFn; | ||||||||||||||||
assignExperiment: AssignExperimentFn; | ||||||||||||||||
} | ||||||||||||||||
|
||||||||||||||||
const ExperimentationContext = | ||||||||||||||||
createContext<CompassExperimentationProviderContextValue>({ | ||||||||||||||||
useAssignment() { | ||||||||||||||||
return { | ||||||||||||||||
assignment: null, | ||||||||||||||||
asyncStatus: null, | ||||||||||||||||
error: null, | ||||||||||||||||
isLoading: false, | ||||||||||||||||
isError: false, | ||||||||||||||||
isSuccess: true, | ||||||||||||||||
}; | ||||||||||||||||
}, | ||||||||||||||||
assignExperiment() { | ||||||||||||||||
return Promise.resolve(null); | ||||||||||||||||
}, | ||||||||||||||||
}); | ||||||||||||||||
|
||||||||||||||||
// Provider component that accepts MMS experiment utils as props | ||||||||||||||||
export const CompassExperimentationProvider: React.FC<{ | ||||||||||||||||
children: React.ReactNode; | ||||||||||||||||
useAssignment: UseAssignmentHookFn; | ||||||||||||||||
assignExperiment: AssignExperimentFn; | ||||||||||||||||
}> = ({ children, useAssignment, assignExperiment }) => { | ||||||||||||||||
// Maintain stable object reference for context value to prevent unnecessary re-renders | ||||||||||||||||
// of consuming components, while keeping the function implementations up-to-date | ||||||||||||||||
const { current: contextValue } = useRef({ useAssignment, assignExperiment }); | ||||||||||||||||
contextValue.useAssignment = useAssignment; | ||||||||||||||||
contextValue.assignExperiment = assignExperiment; | ||||||||||||||||
Comment on lines
+48
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The useRef pattern here won't prevent re-renders as intended. The context value object is recreated on every render because it captures the current prop values. Consider using useMemo with proper dependencies instead:
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ignoring this, see above discussion |
||||||||||||||||
|
||||||||||||||||
return ( | ||||||||||||||||
<ExperimentationContext.Provider value={contextValue}> | ||||||||||||||||
{children} | ||||||||||||||||
</ExperimentationContext.Provider> | ||||||||||||||||
); | ||||||||||||||||
}; | ||||||||||||||||
|
||||||||||||||||
// Hook for components to access experiment assignment | ||||||||||||||||
export const useAssignment = (...args: Parameters<UseAssignmentHookFn>) => { | ||||||||||||||||
return useContext(ExperimentationContext).useAssignment(...args); | ||||||||||||||||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit]: if this package is intended to be used within
compass-telemetry
only then it makes sense to add it here, or else you can add it to base eslint config file: https://github.com/mongodb-js/compass/blob/main/configs/eslint-config-compass/index.jsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we'll need these types outside this package! We'll import
useAssignment
orassignExperiment
, but I don't think we'll have to define any other objects with the types.