[Fix] DisposeResources: continue chain on throw#10
Merged
ljharb merged 2 commits intoes-shims:mainfrom Apr 25, 2026
Merged
Conversation
2f2219a to
21fd452
Compare
ljharb
requested changes
Apr 21, 2026
Member
ljharb
left a comment
There was a problem hiding this comment.
I'm not stoked about the LLM usage here. Either way, the fix (from _ to -) is a good one! Could you please put that, and a regression test, in its own commit, and then put any refactors you're interested in in their own commits?
Step 1.a of the spec short-circuits null/undefined `V` for sync-dispose, returning `UNUSED` without pushing a resource. The hint constant elsewhere is `SYNC-DISPOSE` (hyphen), but the guard here read `SYNC_DISPOSE` (underscore), so the branch was never taken and `use(null)` / `use()` on a sync `DisposableStack` pushed a method-less sync resource onto the stack. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…defer Fixes es-shims#9 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
21fd452 to
f749cdc
Compare
Contributor
Author
|
The typo fix and minimal fix for #9 with their respective regression tests are in their own commits now. |
f749cdc to
089d7af
Compare
Member
|
Thanks. The two fixes look great - the refactors, though, make the AOs deviate farther from the spec text. I try to keep my spec polyfills as close to 1:1 with the spec as possible. |
089d7af to
d533006
Compare
Contributor
Author
|
Removed the refactors from this PR. |
d533006 to
6982b47
Compare
ljharb
approved these changes
Apr 25, 2026
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.
Fixes #9.
Root cause
In
aos/DisposeResources.js, the async path chained each resource's disposal via$then(prev, onFulfilled, rejecter). Two problems:rejecteron the rejected promise, so the earlier-pushed resource's dispose never ran.rejecterreturnedundefined, so the final promise resolved toundefinedinstead of the accumulatedCompletionRecord. The caller'scompletion['?']()then threw the TypeError shown above.Changes
aos/DisposeResources.js: rewrite the async path so every resource's dispose runs regardless of earlier errors, and the promise resolves to the up-to-date completion at the end.aos/AddDisposableResource.js: fix a typo ('SYNC_DISPOSE'->'SYNC-DISPOSE') souse(null)/use(undefined)on a syncDisposableStackcorrectly returnsUNUSED(step 1.a) instead of pushing a method-less sync resource. Lands in its own commit with a regression test.test/tests.js: regression tests for the issue (a throwing later-pushed defer runs the earlier-pushed defer, and a promise-rejecting defer surfaces the rejection) and for theSYNC-DISPOSEtypo.The existing behavior of rejecting mixed sync/async stacks is unchanged.
Test plan
npm test(runs lint + 822 tape assertions, 812 pre-existing + 10 new) passes.1is printed, thenError: 2is thrown):