-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
chore: migrate jest-resolve to TypeScript #7871
Conversation
1009ef2
to
2fdd92b
Compare
}, | ||
"devDependencies": { | ||
"@types/browser-resolve": "^0.0.5", | ||
"jest-haste-map": "^24.0.0" | ||
}, | ||
"peerDependencies": { | ||
"jest-haste-map": "^24.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes the most sense - the constructor of jest-resolve
takes ModuleMap
from jest-haste-map
. So both for types but also for usage, we need to ensure that the API is what we expect it to be
Codecov Report
@@ Coverage Diff @@
## master #7871 +/- ##
==========================================
- Coverage 58.41% 58.14% -0.28%
==========================================
Files 178 174 -4
Lines 6630 6424 -206
Branches 5 6 +1
==========================================
- Hits 3873 3735 -138
+ Misses 2755 2687 -68
Partials 2 2 Continue to review full report at Codecov.
|
import userResolver from '../__mocks__/userResolver'; | ||
import nodeModulesPaths from '../nodeModulesPaths'; | ||
import defaultResolver from '../defaultResolver'; | ||
import {ResolverConfig} from '../types'; | ||
|
||
// @ts-ignore: types are wrong. not sure how... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
built haste-map index.d.ts
:
import HasteFS from './HasteFS';
import HasteModuleMap, { SerializableModuleMap as HasteSerializableModuleMap } from './ModuleMap';
export declare type ModuleMap = HasteModuleMap;
export declare type SerializableModuleMap = HasteSerializableModuleMap;
export declare type FS = HasteFS;
totally missing the main export.
Working on how to fix this atm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we can fix this without dropping the export type
s at the top of haste map. Basically, we want both export = HasteMap
(because making it the default export like it should be would be breaking) and export type ...
. It would be fine for a normal property (can just put it on the class), but I don't know how we could possibly put types onto HasteMap
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for digging! I say we keep it as is then, and try to go for a real ESM run at some point in the future.
It's easier with modules like haste map anyways since it's mostly internal and not pluggable like other parts of Jest are
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Guess the ModuleMap
and HasteFS
types are more important anyway, they're the ones used in Context
etc
b5dbcc1
to
fc98b0d
Compare
fc98b0d
to
7d70b2f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome
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
As expected, had to tweak
jest-haste-map
's types a bit to matchundefined | null
stuff. Can revisit later and be more consistent across the boardBuilt diff:
Test plan
Green CI