Conversation
|
For maintainers only:
|
| /** @private @type {SortableSet<Module>} */ | ||
| this._modules = new SortableSet(undefined, sortByIdentifier); | ||
| /** @private */ | ||
| /** @private @type {SortableSet<ChunkGroup | WithId>} */ |
There was a problem hiding this comment.
Why is | WithId needed? This should be a Set with only ChunkGroups
| * Create a new sortable set | ||
| * @param {Array<T>=} initialIterable The initial iterable value | ||
| * @param {SortFunction=} defaultSort Default sorting function | ||
| * @typedef {(a: T, b: T) => number} SortFunction |
There was a problem hiding this comment.
Put the typedef in a separate comment.
Actually => is not valid jsdoc. You should use @callback or {function(T, T): number}
| this.sortWith(this._sortFn); | ||
| } | ||
|
|
||
| // eslint-disable-next-line valid-jsdoc |
There was a problem hiding this comment.
Same. Use @callback or function(SortableSet<T>): R
| * @param {(instance: SortableSet<T>) => TODO} fn function to calculate value | ||
| * @returns {TODO} returns result of fn(this), cached until set changes | ||
| */ | ||
| getFromCache(fn) { |
There was a problem hiding this comment.
This can use @template R for the return type.
There was a problem hiding this comment.
I think fn should actually be function(SortableSet<T>): T, and the return type of getFromCache should be T. That's because the type of data is T because the type of _cache is Map<Function, T>.
| * @param {Function} fn - function to calculate value | ||
| * @returns {TODO} - returns result of fn(this), cached until set changes | ||
| * @param {Function} fn function to calculate value | ||
| * @returns {any} returns result of fn(this), cached until set changes |
|
@sokra According to the types, we might be using |
| * @returns {TODO} - returns result of fn(this), cached until set changes | ||
| * Get data from cache | ||
| * @param {function(SortableSet<T>): T[]} fn function to calculate value | ||
| * @returns {TODO} returns result of fn(this), cached until set changes |
There was a problem hiding this comment.
I think you can return T from here and getFromUnorderCache, right?
There was a problem hiding this comment.
Thanks. I think T[] is more accurate. It blow up in some other place. Fun to see code inconsistencies when you add types!
ChunkGroup has an Lines 121 to 127 in a1e79ab Is it possible to change |
| source.add("[]"); | ||
| return source; | ||
| } | ||
| /** @type {Module[]} */ |
There was a problem hiding this comment.
That's not correct. The type is {id,source}[].
I think you don't even need a annotation here, typescript should infer the type automatically.
| * @template T item type in set | ||
| */ | ||
| class SortableSet extends Set { | ||
| // eslint-disable-next-line valid-jsdoc |
There was a problem hiding this comment.
remove the eslint-disable comment
|
hmm |
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
|
Thanks |
What kind of change does this PR introduce?
Types
Did you add tests for your changes?
N/A
Does this PR introduce a breaking change?
N/A
What needs to be documented once your changes are merged?
N/A