-
-
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
refactor(jest-haste-map): clean up code and types #12795
Conversation
@@ -444,7 +435,7 @@ export default class HasteMap extends EventEmitter { | |||
let hasteMap: InternalHasteMap; | |||
try { | |||
const read = this._options.resetCache ? this._createEmptyMap : this.read; | |||
hasteMap = await read.call(this); |
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.
await
does nothing here.
if (dedupMap == null) { | ||
dedupMap = Object.create(null) as {}; | ||
if (!dedupMap) { | ||
dedupMap = Object.create(null) as ModuleMapItem; |
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.
Just like similar cases above, here dedupMap
does not have value yet, so {}
is somewhat fine. But the value gets assigned just a couple of lines later. So it makes sense to type it with correct shape already here.
@@ -466,7 +457,7 @@ export default class HasteMap extends EventEmitter { | |||
const setModule = (id: string, module: ModuleMetaData) => { | |||
let moduleMap = map.get(id); | |||
if (!moduleMap) { | |||
moduleMap = Object.create(null) as {}; | |||
moduleMap = Object.create(null) as ModuleMapItem; |
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.
do we need to cast? isn't moduleMap
already the correct type?
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.
Does not work without casting. It needs some type, because Object.create()
returns any
.
forceInBand: boolean; | ||
}): JestWorkerFarm<HasteWorker> | HasteWorker { | ||
private _getWorker( | ||
options = {forceInBand: false}, |
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.
false
, not boolean
?
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.
The idea was to have default value. options
object always has some value, so there is no need for options &&
check later.
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.
oh, right, misread
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. |
Split from #12680
Summary
While pushing forward with #12680, I started noticing few details which can be cleaned up in
jest-haste-map
. Few initial values, nullish coalescing and some TS bits.Test plan
Green CI.