Skip to content

Conversation

kand
Copy link

@kand kand commented Oct 8, 2025

No description provided.

Copy link

changeset-bot bot commented Oct 8, 2025

⚠️ No Changeset found

Latest commit: f59ae46

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

return () => {
sessionRef.current?.destroy();
sessionRef.current = null;
updateSession(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

It is unclear to me if we need to update the session to null here.

In the case that the component is unmounted and this clean up runs, then we're updating state within an unmounted component and will trigger a React warning. I read in this blog post that the warning was removed in React 18, but react-paypal-js is on React 17. Found the actual React Working Group post on it here.

In the case that one of the dependencies has updated and the useEffect is going to rerun, I believe that the previous session will be replaced with the new session before the button re-renders.

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed on slack here to remove this, but I wanted to keep the comment for posterity's sake

Comment on lines +33 to +34
// TODO what if sdk instance is not available? Error?
throw new Error("no sdk instance available");
Copy link
Contributor

Choose a reason for hiding this comment

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

Good question about this. I think there are a few reasons that sdkInstance would be null, like if the hook is being used outside of the Provider for example. I wonder if discussing the error handling strategy overall is a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants