6.x by jdeniau · Pull Request #2071 · immutable-js/immutable-js · GitHub
Skip to content

6.x - #2071

Draft
jdeniau wants to merge 168 commits into
mainfrom
6.x
Draft

6.x#2071
jdeniau wants to merge 168 commits into
mainfrom
6.x

Conversation

@jdeniau

@jdeniau jdeniau commented Mar 19, 2025

Copy link
Copy Markdown
Member

No description provided.

Alexandre VICTOOR and others added 30 commits January 4, 2025 19:00
…able-js into alexvictoor-no-constructor-returns
* Call super()" before this in constructors

* Fix issue with MapIterator re-setting the next method
jdeniau and others added 30 commits August 15, 2026 20:55
Everything in the mixin that does not depend on module-evaluation order
becomes a real typed method on Collection.ts:

- base: toArray, toJSON, entrySeq (replacing its throwing placeholder),
  __toString, __toStringMapper, contains, inspect, toSource
- keyed: mapEntries, mapKeys, plus the toArray/toJSON/__toStringMapper
  narrowings
- indexed/set: toArray/toJSON narrowings, Set#has

Symbol.iterator and Set#keys must stay reference-equal to the iteration
method they delegate to (tested behaviour), so they move as prototype
alias assignments below the classes, with declare members for the type.

isOrdered's guard now intersects the IS_ORDERED_SYMBOL brand it actually
tests: the public OrderedCollection shape (toArray + Symbol.iterator) is
structurally matched by every collection now that the base class types
them, which made negative isOrdered checks narrow plain collections to
never (deepEqual).

methods/toObject.js only backed the keyed toJSON alias and is now
orphaned; delete it. The mixin keeps only the methods that cannot move
yet: the To*Sequence/concat group (importing operations/sequences.ts
from Collection.ts would put two eval-time extends edges in the same
import cycle) and the late-bound concrete-collection conversions.
The base docstrings were carried over with the mixin migration, but the
kind-specific overrides have their own wording in immutable.d.ts
(Keyed: Object form + key stringification; Indexed/Set: Array form).
- concatFactory: the base collection type is statically iterable since
  Symbol.iterator moved out of the mixin onto CollectionImpl, so the
  stale comment is rewritten and the `as unknown as` bridge becomes a
  plain downcast (the remaining lie is only the `unknown` element type).
- entrySeq: the patched `fromEntrySeq` round-trip is a plain downcast
  too (KeyedSeqImpl subtypes the SeqImpl returned by toSeq); the TODO
  documenting the indexed-source lie stays.
- ToIndexedSequence: replace the `?? 0` fallbacks with non-null
  assertions plus a comment, stating the ensureSize invariant instead of
  masking it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WvEjriPc3Rbx3sqApa2coD
Only `this.includes` is off-limits (it would recurse through Set's own
`includes` → `has` delegation); `super.includes` is exactly the wanted
semantics and avoids duplicating the base method body.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WvEjriPc3Rbx3sqApa2coD
Unconditional assignment creates own properties whose value is
`undefined` when the first iterable is unbranded. Today the predicates
test truthiness so this is invisible, but the `in`-based checks planned
for 6.0 (see the isOrdered @ts-expect-error note) would read such a
property as branded, making every ConcatSeq look keyed, indexed and
ordered at once.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WvEjriPc3Rbx3sqApa2coD
The `& { [IS_ORDERED_SYMBOL]: true }` intersection was repeated across
the three guard signatures; a named OrderedBrand alias carries the
rationale once and keeps the signatures readable.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WvEjriPc3Rbx3sqApa2coD
…es-to-ts

Migrate operations/sequences.js to TypeScript and move the order-safe mixin methods onto the classes
The mixin entries in CollectionImpl.js only existed because Collection.ts
cannot import operations/sequences.ts without a cycle. Installing the
methods from sequences.ts itself keeps them next to the wrapping sequence
classes they build, fully typed, and shrinks the mixin further.

Collection.ts now declares the public types: fromEntrySeq joins the
throwing placeholders and concat gets its base d.ts signature, which also
makes the existing *SeqImpl.concat declare-narrowings valid against a real
(bivariant) base method.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EJWDpKjoLcWomxKD3WEmvd
Snapshot of migrated vs remaining files, the mixin dismantling state, the
TODO [TS-MIGRATION] inventory, the source-pass type-test skip list, and
the findings of a behavioural JS->TS consistency review (recorded here,
to be addressed in follow-up PRs).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EJWDpKjoLcWomxKD3WEmvd
Per review on the follow-up PR: the kind narrowings from the d.ts land
with the migration itself instead of a deferred TODO. The three
*CollectionImpl classes get declare properties (type-only, valid against
the base bivariant method), and SeqImpl redeclares concat as a real
method for the public Seq contract — a declare property there would
break the structural *SeqImpl -> SeqImpl assignability — overwritten at
runtime by operations/sequences.ts like the base one, via a new
Seq-typed concatFactory overload.

Un-skips the 'Set concat' source-pass type test.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EJWDpKjoLcWomxKD3WEmvd
The end goal is to emit the public .d.ts from the sources, so migrated
types must stay as close as possible to immutable.d.ts and may only
deviate in the stricter direction (tighter types, or new type
parameters replacing unknown/any). Refresh the status report
accordingly.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EJWDpKjoLcWomxKD3WEmvd
…igration-bctlmg

Move sequence methods from mixin and add TS-migration status report
countByFactory/groupByFactory keep their runtime bodies; the Map they
build is typed through the d.ts import until Map.js is migrated. The
countBy/groupBy mixin entries move onto the prototype from
aggregations.ts itself — same pattern as operations/sequences.ts — with
throwing placeholders carrying the public d.ts types on CollectionImpl.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EJWDpKjoLcWomxKD3WEmvd
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EJWDpKjoLcWomxKD3WEmvd
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EJWDpKjoLcWomxKD3WEmvd
…gations-to-ts

Migrate operations/aggregations.js to TypeScript
Each finding from .agents/migration-status.md was traced back to its
originating PR, then fixed or confirmed as an intentional change:

- R1 (isSubset string argument, from #2204): keep the new
  character-iteration semantics, consistent with isSuperset and with
  Collection('abc'); documented as a v6 breaking change in the CHANGELOG
  and pinned by multi-char unit tests.
- R2 ('@@iterator' fallback removal, from #2127): confirmed intended
  modernisation; explicit CHANGELOG entry, and the two immutable.d.ts
  docstrings no longer mention @@iterator. immutable.js.flow is left
  untouched since Flow's @@iterator syntax denotes Symbol.iterator.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0122shfP2ngkkjuXZBWoeFGW
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0122shfP2ngkkjuXZBWoeFGW
…-yzlr2p

Resolve the three remaining likely-bugs findings (R1-R4)
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.

5 participants