fix: make the reused-tag timeout configurable, drop the prefetch - #56
sidgaikwad wants to merge 2 commits into
Conversation
|
@sidgaikwad is attempting to deploy a commit to the Unlayer Team on Vercel. A member of the Team first needs to authorize it. |
lucasbesen
left a comment
lucasbesen
left a comment
There was a problem hiding this comment.
I don’t think we should merge this as written.
On the normal mount path, createEditor() runs immediately after await loadScript() and starts the bundle request itself. Prefetching here moves that request slightly earlier; it doesn’t eliminate an additional network round trip.
It also changes version selection for host-installed embeds. With embed.js already evaluated and no bundle loaded, passing options={{ version: '2.6.0' }} loads 2.6.0 on the base branch but 2.7.0 on this branch. The new load() call selects “latest” first, and the cached promise prevents createEditor() from honoring the pin. The mocked tests don’t catch this.
Please remove the prefetch change, or demonstrate a meaningful performance improvement while preserving explicit version selection and adding regression coverage.
The timeout parameter is reasonable internal cleanup, but it remains inaccessible to package consumers, so it doesn’t address consumer configurability yet.
sidgaikwad
changed the title
The load listener prefetches the versioned bundle so the first createEditor does not pay a second round trip, but the early return for an already-installed window.ImageEditor bypassed it entirely. So on exactly the pages the tag-reuse logic exists to support — where the host injected embed.js itself — the prefetch never happened and the first mount was a full round trip slower than the injected path. The embed loader caches its own promise, so the duplicate call is a no-op when the host already triggered it. Also make the reused-tag timeout a parameter (defaulting to the exported REUSED_TAG_TIMEOUT_MS) rather than a hardcoded constant, so the bound is overridable and directly testable.
Removes the load() prefetch on the already-installed path. It did not eliminate a round trip: createEditor is awaited immediately after loadScript resolves and starts the bundle request itself, so the prefetch bought a microtask. Upstream already prefetches on the path where it does help, inside the load listener. It also risked overriding version selection. load() with no arguments resolves "latest" and the embed caches that promise, so a version the embed's own createEditor pins could be silently ignored on a page that installed embed.js itself. A test now pins the no-prefetch behaviour so this cannot be reintroduced by accident. Keeps the reused-tag timeout work and finishes it: the parameter was internal-only, so package consumers still could not configure it. Adds a reusedTagTimeoutMs prop, read from latestPropsRef at call time so changing it never remounts the editor, and documents it in the props table.
246a5da to
beaff1b
Compare
2 participants
Addresses the
reusedTagTimeoutMshalf of #44. The prefetch half is dropped — see below.What changed since review
@lucasbesen was right on both counts, and I verified each before acting.
The prefetch is gone
Two independent reasons:
createEditoris awaited immediately afterloadScriptresolves and starts the bundle request itself, so the prefetch bought a microtask. Upstream already prefetches on the path where it genuinely helps — inside theloadlistener, where there is a gap.load()with no arguments resolves"latest"and the embed caches that promise, so a version the embed's owncreateEditorpins could be silently ignored on a page that installedembed.jsitself.One correction to the review for the record:
MountOptionshas noversionfield in this repo, sooptions={{ version: '2.6.0' }}wouldn't typecheck as written. The mechanism is real regardless — a cached"latest"promise defeats any pin applied downstream — so the conclusion stands.A test now pins the no-prefetch behaviour so it can't be reintroduced by accident:
I did not try to "demonstrate a meaningful performance improvement" because I don't believe there is one to demonstrate.
The timeout is now actually reachable
You were right that this didn't address consumer configurability —
loadScripttook the parameter butImageEditorcalledloadScript(scriptUrl)and nothing exposed it.Adds a
reusedTagTimeoutMsprop, documented in the README props table. It's read fromlatestPropsRefat call time rather than captured in the effect closure, so changing it never remounts the editor — covered by a test.Verification
lint,typecheck,buildcleanloadScriptsignatureNote on #44
If this merges as-is, #44 is only half resolved — the prefetch half is being declined rather than implemented. Happy to close #44 with that noted, or leave it open if you'd rather revisit the host-injected path separately.