-
-
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
fix(jest-types): make ConfigGlobal
an interface
#9570
Conversation
packages/jest-types/src/Config.ts
Outdated
@@ -27,7 +27,9 @@ export type HasteConfig = { | |||
export type ReporterConfig = [string, Record<string, unknown>]; | |||
export type TransformerConfig = [string, Record<string, unknown>]; | |||
|
|||
export type ConfigGlobals = Record<string, any>; | |||
export interface ConfigGlobals { | |||
[K: string]: any; |
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.
[K: string]: any; | |
[K: string]: unknown; |
?
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.
That'll be the ideal; let's commit and see if anything breaks :D
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 also wonder if we should call it global
instead of K
?
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.
K
is typically the convention for this pattern, as it's a generic that represents the property name.
i.e
/**
* Construct a type with a set of properties K of type T
*/
type Record<K extends keyof any, T> = {
[P in K]: T;
};
I'm not fussed, but the actual name won't show up much anyway.
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.
It's not really generic though, it's a string. but it doesn't matter like you say 🙂
seems this introduced a type error in normalize
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.
Ah so the proper type should be object
, due to:
const mergeGlobalsWithPreset = (
options: Config.InitialOptions,
preset: Config.InitialOptions,
) => {
if (options['globals'] && preset['globals']) {
for (const p in preset['globals']) {
options['globals'][p] = {
...preset['globals'][p],
...options['globals'][p],
};
}
}
};
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.
@SimenB so we've got a couple of options, depending on how you're feeling:
- we stick with
any
, - we adjust
mergeGlobalsWithPreset
to do a typecheck to ensureoptions['globals'][p]
is an object - we refactor this away: https://github.com/facebook/jest/blob/f47ade4edb112261e14be459185e2396d9c8cbfc/packages/jest-config/src/ValidConfig.ts#L54
- we use a cast or ignore
Looking at the docs, I think any
maybe the "correct" type?
A set of global variables that need to be available in all test environments.
For example, the following would create a global
__DEV__
variable set totrue
in all test environments:
Poking around a bit in normalize
, I can't actually find what stops jest from crashing with an error about ...
?
Today I learned that you can actually spread a boolean without erroring O.o
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.
To me it looks like the code is broken, and mergeGlobalsWithPreset
will discard e.g. the __DEV__: true
example from the docs? It should use a proper recursive merge, not blindly spread into one another
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.
Agreed. I'll adjust the implementation.
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.
packages/jest-types/src/Config.ts
Outdated
@@ -27,7 +27,9 @@ export type HasteConfig = { | |||
export type ReporterConfig = [string, Record<string, unknown>]; | |||
export type TransformerConfig = [string, Record<string, unknown>]; | |||
|
|||
export type ConfigGlobals = Record<string, any>; | |||
export interface ConfigGlobals { | |||
[K: string]: object; |
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 unknown
is correct, we just need to properly merge the config provided rather than blindly spreading
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.
Yeah for sure - I've got a test and everything; just need to come up with a merging solution.
One of the jest packages already has one coded but not exported - I could move that into a shared package.jest-utils
(iirc that's a package that exists?)
Otherwise deepMerge
is used by a few packages, so I'll probably just stick that there 🤷♂
Co-Authored-By: Simen Bekkhus <sbekkhus91@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #9570 +/- ##
==========================================
- Coverage 65.09% 65.09% -0.01%
==========================================
Files 287 287
Lines 12145 12144 -1
Branches 3007 3009 +2
==========================================
- Hits 7906 7905 -1
Misses 3604 3604
Partials 635 635
Continue to review full report at Codecov.
|
ended up being a bug fix, nice 😀 |
It does indeed :/ |
ConfigGlobal
an interaceConfigGlobal
an interface
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. |
As discussed here.
Summary
Makes
ConfigGlobal
an interface to allow for declaration merging:Test plan
None 😬