types(jest-haste-map): Expose a minimal public API to TypeScript #13023
Conversation
robhogan
force-pushed
the
types/restricted-interfaces
branch
from
a4acf85 to
ee77bcc
Compare
|
oops!😅 solving the same wheel closing mine @robhogan ? |
|
@robhogan sorry, missed this one! feel free to ping me, I'm always up for reviewing PRs from Meta peeps 🙂 I merged in |
| import Runtime from 'jest-runtime'; | ||
|
|
||
| export default function createContext( | ||
| config: Config.ProjectConfig, | ||
| {hasteFS, moduleMap}: HasteMapObject, | ||
| {hasteFS, moduleMap}: {hasteFS: IHasteFS; moduleMap: IModuleMap}, |
There was a problem hiding this comment.
why do we have an inline type here instead of using a named one?
There was a problem hiding this comment.
I mostly meant, why is this HasteMapObject thing not exported from jest-haste-map?
| @@ -109,11 +111,12 @@ type Watcher = { | |||
|
|
|||
| type HasteWorker = typeof import('./worker'); | |||
|
|
|||
| export type {default as FS} from './HasteFS'; | |||
| export {default as ModuleMap} from './ModuleMap'; | |||
| export const ModuleMap = HasteModuleMap as { | |||
robhogan
changed the title
| @@ -1143,3 +1146,11 @@ function copy<T extends Record<string, unknown>>(object: T): T { | |||
| function copyMap<K, V>(input: Map<K, V>): Map<K, V> { | |||
| return new Map(input); | |||
| } | |||
|
|
|||
| // Export the smallest API surface required by Jest | |||
| type IJestHasteMap = HasteMapStatic & { | |||
There was a problem hiding this comment.
I'm not sure to be honest. In a way it's exposed via typeof the default export. Users of hasteImplModulePath shouldn't be using it, because they only need to provide a module that implements the smaller HasteMapStatic interface (which is exported).
The general idea is to "hide" statics we don't need to be public - such as the bunch that come from extending EventEmitter.
Happy to go with your judgement anyway.
There was a problem hiding this comment.
fair enough 👍 If for some reason people want it, I assume they'll send a PR 😀
| ChangeEvent, | ||
| HasteMap as HasteMapObject, |
There was a problem hiding this comment.
removing these exports is a breaking change
There was a problem hiding this comment.
specifically HasteMapObject probably - if people get a newer version of jest-haste-map than @jest/core it might cause compilation issues.
But maybe not - since we bundle our TS types, lots of internal types are never present in d.ts files - https://www.runpkg.com/?@jest/core@29.0.2/build/index.d.ts
There was a problem hiding this comment.
Yeah, I think this is safe, because as you say it's not referenced by the built declaration files there should be no observable change to consumers of jest.
(It's type-breaking to users of jest-haste-map directly of course, but I'm working on the assumption that Jest semver is at a higher level.)
|
Thanks for looking at this @SimenB (but have a good weekend! 😄) |
same to you! 👍 |
|
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
Currently,
jest-haste-mapexposes a large API surface, most of which is unused by Jest and none(?) of which is documented. Some of this is a legacy of internal FB usage.This is a conservative type-only change to minimally define the interfaces actually used by Jest itself. It's intended to be particularly useful to users of
hasteMapModulePath, to reduce and clarify the API they're required to implement, and at Meta it will help us flag uses of APIs Jest doesn't need.(This also paves the way for upstreaming some of the work we're doing/intend to do on
metro-file-map(context: facebook/metro#812) to extract and decouple some Meta-specific stuff)Test plan
No runtime changes, TS build passes.