Refactor PayPalProvider #714
Conversation
|
| }); | ||
| } | ||
| }, [state.loadingStatus]); // Run when loadingStatus changes, but only act once | ||
| let isSubscribed = true; |
There was a problem hiding this comment.
Initially this useEffect did not leverage the isSubscribed strategy, when running the unit tests I was seeing warnings related to not preventing state updates on unmounted components.
Without the isSubscribed strategy, technically this useEffect could run setPaypalNamespace on an unmounted component.
I know that there is some debate about whether we need the isSubscribed strategy, I fall on the side of we should prevent state updates or dispatches to unmounted components and avoid the subsequent console warnings (courtesy of React), until we can upgrade the React version to 18/19. Check out this React Working Group post on removing this warning from React 18.
| const instance = await paypalNamespace.createInstance( | ||
| memoizedCreateOptions, | ||
| ); | ||
| const instance = await paypalNamespace.createInstance({ |
There was a problem hiding this comment.
Do we need a !isSubscribed check before this line? Is it possible for the component to have unmounted between calling the parent useEffect and the start of the async function, before createInstance has been called?
There was a problem hiding this comment.
I don't think we need one. Everything in the useEffect runs synchronously and must complete before the clean up method can be called b/c of an unmounting. So createInstance will always be called before the clean up can be called, but the async operation may complete after both the useEffect completes AND the component has unmounted.
| let isSubscribed = true; | ||
|
|
||
| const loadSdk = async () => { | ||
| try { |
There was a problem hiding this comment.
Similarly, here, do we need an !isSubscribed check at the top of this async function?
There was a problem hiding this comment.
Same as above, I don't think we need to update here.
EvanReinstein
force-pushed
the
feature/DTPPCPSDK-3933-update-react-provider
branch
from
9ce7952 to
574257b
Compare
EvanReinstein
force-pushed
the
feature/DTPPCPSDK-3933-update-react-provider
branch
from
574257b to
c333d0e
Compare
EvanReinstein
deleted the
feature/DTPPCPSDK-3933-update-react-provider
branch
The provider had a few todos/code smells that were not taken up during the first pass. This updates the following:
scriptOptionsprop to its 2 keys.componentsarray specfically.dequalpackage from dependencies and replaces with a customcomponentsspecificisEqualvariation.useEffectTakes some of the ideas that were put together in this PR