collapse redundant type unions in newContent JSDoc - #161
Merged
Conversation
Element is a subtype of Node, so `Element | Node | ...` in the two
public newContent annotations is just `Node | ...`.
normalizeParent's annotation carried a third redundancy: `Document &
{generatedByIdiomorph:boolean}` is likewise absorbed by Node, and the
property it names does not exist -- generatedByIdiomorph is a WeakSet,
not a field. It was added in e4eeb8a to satisfy tsc, after the WeakSet
landed in 7f07002; the cast on the `.has()` call covers it now.
No behavioural or type-level change: the annotations denote exactly the
same types. Relates to bigskysoftware#103.
Collaborator
|
@myabc Love this. As I said, I'm not a user of typescript, so I really appreciate you finding things like this and applying your best judgement. I also appreciate the input you're making on the other issues and PRs, so thank you! |
botandrose
marked this pull request as ready for review
August 26, 2026 09:09
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
2 participants
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.
Small type-hygiene cleanup, no behaviour change. Prompted by #103, which asks whether the
newContentpossibility space can be pared down — part of it turns out to be double-counted already.Element | Nodeis justNodeElementextendsNode, so the union in both publicnewContentannotations denotes exactly the same type with or without theElementarm:#103 lists six accepted types; five of them are distinct. Worth knowing before deciding what to remove — and
HTMLCollection/Node[]are both just node iterables, so the count of genuinely distinct shapes is lower still.A dead intersection in
normalizeParentDocument & {generatedByIdiomorph: boolean}is redundant twice over. It is absorbed byNodelike the arms above, and the property it names does not exist —generatedByIdiomorphis aWeakSet(src/idiomorph.js:1216), never a field on a document.It looks vestigial rather than intentional. The WeakSet landed in 7f07002 (2024-11-23); the intersection was added later in e4eeb8a (2025-01-14), titled "satisfy tsc". The
/** @type {Element} */cast on the.has()call covers that now, so removing it is clean.Verification
npm run typechecksilent,npm run format:checkclean, chromium suite 182 passed / 0 failed, coverage 100 %, andnode test/lib/ensure-full-coverage.jsexits 0.Since the annotations denote identical types, the generated
dist/*.d.tsare unaffected in substance and no consumer can observe the change.Note
Marked draft — happy to close it if this is churn rather than cleanup, or to fold it into whatever comes out of #103. It merges cleanly against both #155 and #156 (checked), so it should not get in their way either.