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
feat(jest-mock): allow jest.replaceProperty
to replace undefined
or nonexistent properties
#13958
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -177,7 +177,6 @@ export interface Replaced<T = unknown> { | |
* Restore property to its original value known at the time of mocking. | ||
*/ | ||
restore(): void; | ||
|
||
/** | ||
* Change the value of the property. | ||
*/ | ||
|
@@ -1332,16 +1331,21 @@ export class ModuleMocker { | |
T extends object, | ||
K extends PropertyLikeKeys<T>, | ||
V extends T[K], | ||
>(object: T, propertyKey: K, value: V): Replaced<T[K]> { | ||
if (object === undefined || object == null) { | ||
>( | ||
object: T, | ||
propertyKey: K, | ||
value: V, | ||
options?: {tolerateUndefined?: boolean}, | ||
): Replaced<T[K]> { | ||
if (object == null) { | ||
throw new Error( | ||
`replaceProperty could not find an object on which to replace ${String( | ||
propertyKey, | ||
)}`, | ||
); | ||
} | ||
|
||
if (propertyKey === undefined || propertyKey === null) { | ||
if (propertyKey == null) { | ||
throw new Error('No property name supplied'); | ||
} | ||
|
||
|
@@ -1359,14 +1363,18 @@ export class ModuleMocker { | |
descriptor = Object.getOwnPropertyDescriptor(proto, propertyKey); | ||
proto = Object.getPrototypeOf(proto); | ||
} | ||
if (!descriptor) { | ||
if (!descriptor && !options?.tolerateUndefined) { | ||
throw new Error(`${String(propertyKey)} property does not exist`); | ||
} | ||
if (!descriptor.configurable) { | ||
|
||
if (descriptor?.configurable === false) { | ||
throw new Error(`${String(propertyKey)} is not declared configurable`); | ||
} | ||
if (descriptor?.writable === false) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hm.. What to do with non-writable values like |
||
throw new Error(`${String(propertyKey)} is not declared writable`); | ||
} | ||
|
||
if (descriptor.get !== undefined) { | ||
if (descriptor?.get !== undefined) { | ||
throw new Error( | ||
`Cannot mock the ${String( | ||
propertyKey, | ||
|
@@ -1376,7 +1384,7 @@ export class ModuleMocker { | |
); | ||
} | ||
|
||
if (descriptor.set !== undefined) { | ||
if (descriptor?.set !== undefined) { | ||
throw new Error( | ||
`Cannot mock the ${String( | ||
propertyKey, | ||
|
@@ -1386,7 +1394,7 @@ export class ModuleMocker { | |
); | ||
} | ||
|
||
if (typeof descriptor.value === 'function') { | ||
if (typeof descriptor?.value === 'function') { | ||
throw new Error( | ||
`Cannot mock the ${String( | ||
propertyKey, | ||
|
@@ -1406,7 +1414,7 @@ export class ModuleMocker { | |
object, | ||
propertyKey, | ||
); | ||
const originalValue = descriptor.value; | ||
const originalValue = descriptor?.value; | ||
|
||
const restore: ReplacedPropertyRestorer<T, K> = () => { | ||
if (isPropertyOwner) { | ||
|
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 don't really like the name
tolerateUndefined
. I don't have any better suggestions off the top of my head, tho.tolerateMissing
,allowMissing
,setMissing
... neither sounds good, buttolerates
is even worse imo 😅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.
What about
respectUndefined
? Not sure. Hm.. Did you see my comment on non-writable values? What about optionstrict: false
which would allow creating undefined props and overwriting non-writable values? Simple thing.The target property cannot be
undefined
and non-writable at the same time. So in a way one option which turns off precautions should be enough.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
replaceProperty
actually disallow existing butundefined
properties? I would consider this a bug.And for non-existing properties, it probably makes more sense to add
jest.addProperty
instead which would act likereplaceProperty
+tolerateUndefined
.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 thinking together. How do you see replacement of non-writeable props? Should it just work or do we need an option?
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 for mocking, replacing non-writeable is fine (though they should remain non-writeable). I view non-writeable more as a hint for the runtime behavior while testing setup comes before the actual runtime.
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 don't think I want
addProperty
- then I think we'd need aremoveProperty
as well. It might have been better forreplaceProperty
to have been calledsetProperty
, then taken options for replacement strategy. Anyways, I think a single API onjest
is enough.