chore: migrate jest-haste-map to TypeScript #7854
Conversation
SimenB
force-pushed
the
ts-haste-map
branch
2 times, most recently
from
833b737
to
b73d91e
Compare
| SIZE: 2; | ||
| VISITED: 3; | ||
| DEPENDENCIES: 4; | ||
| SHA1: 5; |
There was a problem hiding this comment.
This is missing in the flow types. It's awesome to have them properly co-located so we discover that stuff
|
|
||
| import {HType} from './types'; | ||
|
|
||
| const constants: HType = { |
There was a problem hiding this comment.
Explicitly typing this ensure the type remains narrow rather than expanded to number
|
@thymikee Feel free to look into the remaining type errors if you've got some time 🙂 |
| map: $Call<typeof Array.from, $Call<$PropertyType<ModuleMapData, 'entries'>>>, | ||
| mocks: $Call<typeof Array.from, $Call<$PropertyType<MockData, 'entries'>>>, | ||
| rootDir: string, | ||
| duplicates: [string, ValueType<DuplicatesIndex>]; |
There was a problem hiding this comment.
duplicates has this signature: Map<string, Map<string, Map<string, number>>>. We'd need to call Array.from recursively if we wanted to properly toJSON and fromJSON it. Is this even working properly now? To me it looks like we loose all duplicates data on every serialization
There was a problem hiding this comment.
I agree it seems like this isn't working properly. Should probably investigate
SimenB
changed the title
| supportsNativePlatform: boolean | null, | ||
| type: HTypeValue | null, | ||
| ): string | null { | ||
| if (type == null) { |
There was a problem hiding this comment.
if the types are right, then
| if (type == null) { | |
| if (type === null) { |
There was a problem hiding this comment.
We only do getModule (outside of unit tests) right below: return this.getModule(name, platform, null, H.PACKAGE);
So I wonder if it's never null either? May be that FB uses it internally though, who knows
| @@ -66,13 +62,13 @@ export default class ModuleMap { | |||
|
|
|||
| getPackage( | |||
| name: string, | |||
| platform: ?string, | |||
| supportsNativePlatform: ?boolean, | |||
There was a problem hiding this comment.
not sure why we pass null to getModule instead of using this argument
There was a problem hiding this comment.
yea, just wanted to comment about it. maybe we should start passing it?
| @@ -996,6 +1002,7 @@ class HasteMap extends EventEmitter { | |||
| } | |||
|
|
|||
| end(): Promise<void> { | |||
| // @ts-ignore: TODO TS cannot decide if `setInterval` and `clearInterval` comes from NodeJS or the DOM | |||
There was a problem hiding this comment.
this is super weird - setInterval returns NodeJS.Timeout, but clearInterval expects number. Not sure what's up?
| let dependencies: WorkerMetadata['dependencies']; | ||
| let id: WorkerMetadata['id']; | ||
| let module: WorkerMetadata['module']; | ||
| let sha1: WorkerMetadata['sha1']; |
There was a problem hiding this comment.
in theory not needed, but it was way easier to track down where the assignment was wrong that it telling med the return at the bottom was wrong
|
Good job! And sorry I didn't have time to do it yesterday :| |
|
No worries 🙂 Probably a bunch of things that can be improved if you feel up for it! |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
@jeysal I'll open up a PR even though this isn't ready. I've left tests alone except for fixing imports so they pass
See the type build on CI for a list of failures
Build diff:
Test plan
Green CI