Minor cleanup to symbolWalker #18549
Conversation
ghost
force-pushed
the
symbolWalker
branch
3 times, most recently
from
c7f4f99 to
6acb418
Compare
| @@ -994,11 +994,6 @@ namespace ts { | |||
|
|
|||
| /** | |||
| * Gets the owned, enumerable property keys of a map-like. | |||
| * | |||
| * NOTE: This is intended for use with MapLike<T> objects. For Map<T> objects, use | |||
There was a problem hiding this comment.
Comment outdated; Map<T> is now a real ES6 map, so we would use the .keys() method.
|
FYI @weswigham, since this is his code. 😄 |
|
What problem is this solving? |
| @@ -1011,6 +1006,17 @@ namespace ts { | |||
| return keys; | |||
| } | |||
|
|
|||
| export function getOwnValues<T>(sparseArray: T[]): T[] { | |||
| const values: T[] = []; | |||
| for (const key in sparseArray) { | |||
There was a problem hiding this comment.
I'm sure I'm missing something, but how is this different from a for-of loop?
There was a problem hiding this comment.
A for-in loop iterates over the keys of an object, while a for-of loop only works on an object implementing the iterator protocol. See MDN
There was a problem hiding this comment.
It's also supposedly identical to Object.values, which, much like hasOwnProperty, which we should probably check for the existence of and use if available.
|
@RyanCavanaugh This PR is in lieu of code review, since it was easier to just do it than to try to explain how to do it. |
There was a problem hiding this comment.
I'm generally fine with the spare array stuff; it was using an old-style map-like prior to being updated for the latest compiler, which is essentially the same thing. The inlining I agree less with - the functions are separate for readability, documentation, and logical separation of concerns; I'd prefer not to inline things juts because it's only one line and do away with all that.
| @@ -1011,6 +1006,17 @@ namespace ts { | |||
| return keys; | |||
| } | |||
|
|
|||
| export function getOwnValues<T>(sparseArray: T[]): T[] { | |||
| const values: T[] = []; | |||
| for (const key in sparseArray) { | |||
There was a problem hiding this comment.
It's also supposedly identical to Object.values, which, much like hasOwnProperty, which we should probably check for the existence of and use if available.
src/compiler/symbolWalker.ts
Outdated
| @@ -66,43 +73,22 @@ namespace ts { | |||
| } | |||
| } | |||
| if (type.flags & TypeFlags.TypeParameter) { | |||
| visitTypeParameter(type as TypeParameter); | |||
| visitType(getConstraintFromTypeParameter(type as TypeParameter)); | |||
There was a problem hiding this comment.
I do not really like the inlining of these short functions - it removes the ability to grep for the symbol handling that case, and makes the block less uniform (and potentially makes the outer function more polymorphic). TBQH, if TypeFlags wasn't a flags enum, I'd just have stored these visitors in a map and dispatched on kind; but you can't so 🤷♂️ .
Instead of reviewing #17844 I thought I'd just make a pull request implementing some changes.
forEachinstead of a null check followed byfor-of; this reducesvisitTypeListto a single line, so inlined it.