-
Notifications
You must be signed in to change notification settings - Fork 4
feat: allow context to be passed as null #29
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
base: chore_update_some_packages
Are you sure you want to change the base?
feat: allow context to be passed as null #29
Conversation
8c3b8a1 to
465970b
Compare
0d1892b to
8110063
Compare
465970b to
3ef0c45
Compare
caal-15
left a comment
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.
Hi! I think this technically achieves what we want but added some notes on how it maybe could be improved (at least in my opinion)!
Also, do note that this should be a breaking change, since some people may be depending on the context coming from useTreatment never being null :)
| const sdk = context | ||
| ? context["_sdk"] | ||
| : new absmartly.SDK({ retries: 5, timeout: 3000, ...sdkOptions }); | ||
| const sdk: SDK = |
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.
[Question]: Wouldn't it be better to also keep the SDK null until a non-null context is passed (or requested via resetContext)?
| contextOptions?: ContextOptionsType, | ||
| ) => { | ||
| try { | ||
| if (providedContext == null) { |
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.
[Suggestion]: I think this implies the only method of replacing the null context is via the resetContext function, while this technically works it could create a disconnect between the context being passed to the provider (i.e. null) and the internal context of the provider (an actual fully functional context).
I think a better approach would be to have an useEffect hook listening to possible changes in the context, so that when the context passed to the provider is no longer null we update the internal values, here's a snippet of how it could potentially work:
export const SDKProvider: FC<SDKProviderProps> = ({
sdkOptions,
contextOptions,
context,
children,
}) => {
let contextInitialValue = context;
let sdkInitialValue = context ? context['_sdk'] : null;
if (context === undefined) {
sdkInitialValue = new absmartly.SDK({ retries: 5, timeout: 3000, ...sdkOptions });
contextInitialValue = sdk.createContext(contextOptions);
}
const [context, setContext] = useState<Context | null>(contextInitialValue);
const [sdk, setSdk] = useState<SDK | null>(sdkInitialValue);
useEffect(() => {
if (!!context) {
setContext(context);
setSdk(context['_sdk']);
};
}, [context]);
};You could probably improve performance a bit by memoizing values and such, but this is the gist of it :)
This PR allows context to be passed to the SDKProvider as null. It is then up to the user to reset the context and provide it later. This is an attempt to remove one rerender from static pages on NextJS