-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Migrate CollectionImpl.js to TypeScript with full type declarations #2148
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: 6.x
Are you sure you want to change the base?
Conversation
…meration issue) Co-authored-by: jdeniau <1398469+jdeniau@users.noreply.github.com>
Co-authored-by: jdeniau <1398469+jdeniau@users.noreply.github.com>
Co-authored-by: jdeniau <1398469+jdeniau@users.noreply.github.com>
Co-authored-by: jdeniau <1398469+jdeniau@users.noreply.github.com>
|
@copilot Ok that's interesting, but now when we build, there is a lot of You can check by running |
You're absolutely right. The After investigating several approaches (static imports cause circular dependency initialization errors, dynamic import() would be async), the best solution is to keep the implementation in a separate file that can safely import the circular dependencies. Instead of moving methods to Collection.ts, I'll migrate CollectionImpl.js to TypeScript (CollectionImpl.ts) which:
Working on this approach now in commit [hash]. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: jdeniau <1398469+jdeniau@users.noreply.github.com>
|
@copilot OK but doing that, there is no way that we know that the mixins-defined methods are available in the type. Two possibilities here : Make the type of CollectionImpl defining methods that are in the mixin. Or find a way to solve the circular reference You can check #2105 that tried to solve the circular reference by exploding all methods into smaller files and possibly used late statc binding. |
Co-authored-by: jdeniau <1398469+jdeniau@users.noreply.github.com>
Fixed in 3b8354d. Added TypeScript interface declarations using declaration merging pattern. Now TypeScript knows about all mixin methods:
The interfaces automatically merge with classes of the same name, providing full type information without requiring circular import resolution. All 733 tests pass, IntelliSense works correctly, and there are no Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Migrates
CollectionImpl.jsto TypeScript (CollectionImpl.ts) as part of the 6.x TypeScript migration. The file retains its mixin pattern and module structure to avoid circular dependency issues while gaining full TypeScript type safety through interface declarations.Changes
Core Migration
src/CollectionImpl.jstosrc/CollectionImpl.tsTypeScript Interface Declarations
Added interface declarations in
Collection.tsthat merge with the class definitions to provide full type information:CollectionImpl: 60+ methods (toArray, map, filter, reduce, sort, etc.)KeyedCollectionImpl: flip, mapEntries, mapKeysIndexedCollectionImpl: findIndex, indexOf, splice, zip, interleave, etc.SetCollectionImpl: get, includes, keySeqUses TypeScript's declaration merging pattern where interfaces and classes with the same name automatically merge, providing compile-time type information for runtime-added mixin methods.
Why This Approach
The separate file approach is necessary because:
require()calls in bundled output)Mixin Enhancement
mixin.tsto useObject.getOwnPropertyNames()instead ofObject.keys()Example
The mixin pattern remains unchanged, now with TypeScript and full type declarations:
Impact
require()calls in dist output (bundler properly inlines static imports)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.