Skip to content

Preserve shared Link script while hooks remain mounted - #403

Merged
phoenixy1 merged 1 commit into
masterfrom
fix/shared-script-unmount
Jul 23, 2026
Merged

Preserve shared Link script while hooks remain mounted#403
phoenixy1 merged 1 commit into
masterfrom
fix/shared-script-unmount

Conversation

@phoenixy1

Copy link
Copy Markdown
Collaborator

Concurrent usePlaidLink instances share one Link script, but unmounting any instance removed that script while it was still loading and stranded the remaining hooks. This tracks active script subscribers so cleanup occurs only after the final consumer unmounts, with regression coverage for both paths.

Closes #268.

@phoenixy1

Copy link
Copy Markdown
Collaborator Author

Claude Opus 4.8's review:

What it does. useScript (the forked react-script-hook) cached one <script> per src and, on unmount-while-loading, removed it from the DOM and cache — so when several usePlaidLink hooks shared a still-loading script and any one unmounted, the
script vanished and the surviving hooks never resolved. The fix adds a subscribers refcount on each ScriptStatus: increment in the effect body, decrement in cleanup, and only remove/delete when loading && subscribers === 0. The scripts[src]
=== subscribedStatus guard prevents a stale cleanup from clobbering a freshly-recreated cache entry. Two regression tests cover both paths.

Findings: nothing survived verification.

I traced the paths that usually break this kind of refcount and each is sound:

  • Mount/unmount balance — the increment (index.tsx:111) sits in the effect body after all early-returns, and the decrement (index.tsx:131) is in the returned cleanup. React pairs them 1:1 per src, so the count can't drift negative or leak. The
    error/scriptLoaded early-return at line 75 short-circuits before incrementing and returns no cleanup, so it's balanced (0 in / 0 out).
  • Reuse of loaded/errored scripts — a hook mounting onto an already-loaded status increments then decrements with loading === false, so removal is correctly skipped; an already-errored status early-returns without subscribing. Both correct.
  • src changes — cleanup decrements the old src's status and the new effect increments the new src's; per-src accounting stays balanced.
  • The subscribedStatus alias — capturing status into a const is deliberate and correct: it pins the closure to one status object instead of the mutated outer let, which is what made the old if (status && status.loading) cleanup fire against
    the wrong entry.
  • Tests genuinely regress — both new tests fail against the old cleanup (which unconditionally removed the loading script), and beforeEach resets DOM + scripts, so the shared 'http://scriptsrc/shared' src is isolated.

One pre-existing behavior worth being aware of (not introduced here, not blocking): handleError never sets status.loading = false, so an errored-but-still-"loading" script is removed from DOM/cache when its last subscriber unmounts. That's
arguably the intended "clean slate on error," and it's unchanged by this PR, so it's out of scope.

Clean, minimal, correctly-scoped fix with real regression coverage — no changes requested.

@phoenixy1
phoenixy1 requested a review from dtroupe-plaid July 22, 2026 21:16

@dtroupe-plaid dtroupe-plaid left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM 🎉

@phoenixy1
phoenixy1 merged commit 8f0fac9 into master Jul 23, 2026
1 check passed
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.

Plaid Link returns noop for open function

2 participants