Pass the element to afterEach - #1575
Merged
Merged
Conversation
`afterEach` was called with the error and, on success, the injected SVG. On failure there is neither, so a caller that passed a collection could not say which of its placeholders failed. The error message is not a way back: several of the library's messages do not name their URL, and a URL is not an element in any case, since two placeholders sharing one `data-src` is the normal case for a sprite. `element` is the element that was passed in, and is present on every call, failures included. It is supplied where the errback is wrapped in `svg-injector.ts`, which already holds the element in a closure at both `injectElement` call sites, so the pipeline below it stays per-URL and unchanged. `Errback` served both this option and the internal load callback, so the public half splits off as `AfterEach`. `Errback` keeps its shape and its export. `svg` is written `SVGSVGElement | undefined` rather than `svg?` because a required parameter cannot follow an optional one; in a callback position that is not a breaking change, and existing two-parameter callbacks still typecheck. `examples/error-handling` drops the `afterAll` sweep it needed in order to place its fallbacks, and places each one as its element fails. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
afterEachwas called with the error and, on success, the injected SVG. On failure there is neither, so a caller that passed a collection could not say which of its placeholders failed. The error message is not a way back: several of the library's messages do not name their URL, and a URL is not an element in any case, since two placeholders sharing onedata-srcis the normal case for a sprite.elementis the element that was passed in, and is present on every call, failures included.Notes
svg-injector.tsalready holds the element in a closure at bothinjectElementcall sites, so the argument is supplied where the errback is wrapped.inject-element.ts,load-svg-cached.tsandload-svg-uncached.tsare unchanged, and the load path stays per-URL, which is what it should be given sprites.elementis non-optional, because everyafterEachpath bottoms out in one of those two closures, including the ones that fail before a URL is known.Errbacksplit. One type served both the public option and the internal load callback. Only the public half gains the element, soAfterEachis new andErrbackstays as it was, still exported.svg?becamesvg: SVGSVGElement | undefined, because a required parameter cannot follow an optional one. Not breaking: in a callback position fewer parameters is always assignable, andsvg?already had that type. Checked against@tanem/react-svg, whoseafterEachtakes two parameters and is unaffected.beforeEachdeliberately left alone. It has the same shortcoming, but only fires on the success path, where the SVG that replaced the element already identifies it.Tests
14 new tests in
test/aftereach-element.test.ts, written first, enumerating every path the waycallback-timing.test.tsdoes — success, cache hit, data URL, load failure, rejected content type, missingdata-src, unparseable data URL, unparseable URL, missing symbol, in-flight guard, null parent, a mixed collection, two placeholders sharing one URL, and the single-element branch. The red run confirmed all 14 failed withelementundefined.examples/error-handlingdrops theafterAllsweep it needed in order to place its fallbacks, and places each one as its element fails. It also passes the liveHTMLCollectiondirectly again, since the snapshot only existed to support the sweep.npm testgreen: 402 in the main suite (360 before), 19 in the examples suite, all three browser projects.size-limit4.3 kB against the 5.5 kB budget, so no budget change.Not checked by hand in a browser: nothing here is engine-dependent — one closure variable and a type — and the suite exercises all 14 paths in Chromium, Firefox and WebKit.
make-ajax-request.tsandload-svg-cached.tsare untouched, so thetest/manual/transport checks did not need rerunning.🤖 Generated with Claude Code