Skip to content

fix: only tear down an embed script tag we injected - #49

Open
sidgaikwad wants to merge 2 commits into
unlayer:mainfrom
sidgaikwad:fix/reset-loader-ownership
Open

fix: only tear down an embed script tag we injected#49
sidgaikwad wants to merge 2 commits into
unlayer:mainfrom
sidgaikwad:fix/reset-loader-ownership

Conversation

@sidgaikwad

@sidgaikwad sidgaikwad commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Fixes #32.

Problem

loadScript is deliberately written to cooperate with a page that loads embed.js itself — it returns early when window.ImageEditor already exists, and reuses a host-injected <script> tag rather than injecting a duplicate.

resetLoader did not honour that:

findScriptTag(scriptUrl)?.remove();
delete window.ImageEditor;

It has no notion of who created the tag, and it is called automatically from the component's terminal .catch. So on a page where the host owns the embed, a versioned-bundle load failure inside our component removed a DOM node we never created and deleted a global we do not own — breaking every other consumer of window.ImageEditor on that page, with nothing to indicate why.

Fix

Track injected tags in a module-level WeakSet and gate resetLoader's teardown on it. When we don't own the tag, the reset still evicts our cached promise and rejects pending waiters (so our own state is clean), but leaves the page's DOM and global alone.

A provably-failed tag is still removed either way — both the error event and the reused-tag timeout mean the script will never fire again, so a dead tag is no use to the host, and leaving it in place would make every retry reuse it and time out. That deliberate behaviour and its existing test are unchanged; failWith just takes an explicit removeTag flag now so the reset path can opt out.

One ordering detail: resetLoader captures the tag before calling abort(), since abort may remove it.

Verification

Both new tests are red without the fix:

× resetLoader leaves a host-injected tag and its global alone
× resetLoader rejects waiters on a reused host tag without removing it
  • 3 new tests (host-owned loaded tag, host-owned pending tag, no-op when nothing loaded)
  • All 14 loadScript tests pass; 49 total; coverage still 100% across the board
  • lint, typecheck, build clean

On recovery

An earlier version of this description claimed that not owning the tag meant losing the ability to retry. That was wrong, and the docs in the branch repeated it — both are now fixed.

embed.js does not memoize a failed bundle load:

.catch(function (err) {
  // Don't memoize failure: a transient CDN blip shouldn't kill the
  // image editor for the whole page session — let a reopen retry.
  loadPromise = null;
  throw err;
});

So a later createEditor retries regardless of whether the tag was ours, and there is no permanent-failure mode to trade against. The reset just clears the state this module owns.

Updated in this branch: the resetLoader JSDoc, the call-site comment in ImageEditor.tsx, and the README "Error handling" bullet.

loadScript deliberately cooperates with a page that loads embed.js itself
— it returns early when window.ImageEditor already exists, and reuses a
host-injected tag rather than duplicating it. resetLoader did not: it
removed whatever tag matched the URL and deleted window.ImageEditor
unconditionally.

On such a page a versioned-bundle failure inside one component removed a
DOM node we never created and deleted a global we do not own, breaking
every other consumer of the embed with no indication why.

Track the tags we inject in a WeakSet and gate resetLoader's teardown on
it. A tag that has provably failed (error event or reused-tag timeout) is
still removed either way — a dead tag is no use to the host either, and
leaving it would make every retry reuse it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@vercel

vercel Bot commented Sep 4, 2026

Copy link
Copy Markdown

@sidgaikwad is attempting to deploy a commit to the Unlayer Team on Vercel.

A member of the Team first needs to authorize it.

@lucasbesen

Copy link
Copy Markdown
Member

@sidgaikwad two small things:

  • The trade-off in the description doesn't apply to the current CDN loader: embed.js no longer memoizes a failed bundle load, so a remount still recovers on host-loaded pages. Please update the description and the resetLoader JSDoc so we don't document a permanent-failure mode that doesn't exist.
  • README "Error handling" and the comment above resetLoader in ImageEditor.tsx still describe the unconditional reset. Please align both with the new behavior.

The JSDoc, the call-site comment and the README all justified the reset by
claiming embed.js memoizes a failed bundle load forever, so reloading it
was the only route back. That is not true of the current loader: it nulls
loadPromise in a catch specifically so a transient CDN blip does not kill
the editor for the page session.

So a remount recovers on its own, including on host-loaded pages where the
reset now deliberately leaves the tag and global alone. Describe the reset
as clearing the state we own, not as the mechanism recovery depends on.

No behaviour change.
@sidgaikwad

Copy link
Copy Markdown
Contributor Author

You are right, and I should have checked rather than inheriting the premise from the existing comment. Fixed in a4f2e13.

I pulled the live embed.js (2.4.0) to make sure the replacement text is accurate:

.catch(function (err) {
  // Don't memoize failure: a transient CDN blip shouldn't kill the
  // image editor for the whole page session — let a reopen retry.
  loadPromise = null;
  throw err;
});

So loadPromise is nulled on failure and a later createEditor retries on its own. The permanent-failure mode I described does not exist, on host-loaded pages or ours.

Updated all three places to describe the reset as clearing the state this module owns, rather than as the mechanism recovery depends on:

  • resetLoader JSDoc — dropped the "caches rejections forever" rationale; now ends with "Recovery does not depend on any of this: embed.js nulls its own cached promise when a bundle load fails, so a later createEditor retries whether or not the tag was ours."
  • Call site in ImageEditor.tsx — same correction, and it now says explicitly that on a host-loaded page only our cache is cleared.
  • README "Error handling" — "A later remount retries from scratch after a CDN failure. The wrapper also clears its own loader state, but only tears down a script tag it injected itself..."

PR description rewritten too; the "Trade-off worth reviewing" section is gone and replaced with the correction.

No behaviour change — 49 tests, coverage still 100%, lint/typecheck/build clean.


One follow-up question this raises, which I did not act on.

If embed.js never memoizes a failed load, the resetLoader call in the component's catch may now be doing net harm on the owned-tag path: it deletes a working window.ImageEditor and removes the tag, forcing a full re-download of embed.js, to work around a problem that no longer exists. Plain retrying would recover with less work.

I left it in place because (a) it is a behaviour change beyond this PR's scope, and (b) scriptUrl lets consumers pin an older embed.js that may still memoize failures, so it is arguably a deliberate safety net for those.

Happy to open a separate issue for it, or drop the call in this PR if you would rather — your call, since you know the CDN's version history.

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.

resetLoader removes a host-injected embed script tag and global it does not own

2 participants