fix: add error message when mock is missing export #1819
Conversation
jereklas
changed the title
jereklas
force-pushed
the
add-error-on-missing-mock-exports
branch
from
b324c86
to
d66dc3e
Compare
jereklas
force-pushed
the
add-error-on-missing-mock-exports
branch
from
d66dc3e
to
9d5234e
Compare
| public queueMock(id: string, importer: string, factory?: () => unknown) { | ||
| factory = this.wrapFactoryInProxy(id, importer, factory) |
There was a problem hiding this comment.
Can't we just wrap exports here:
?
There was a problem hiding this comment.
And while we are here, let's throw an error, if factory returns non-object. (With a help message that it should return { default }, if user wants to mock default export)
There was a problem hiding this comment.
I split the proxy objects. The export object proxy is now happening where you suggested, and the factory proxy object is now returning an error if the return value is not an object.
| throw new Error(`[vitest] vi.mock("${id}", factory?: () => unknown) is not returning an object. Did you mean to return an object with a "default" key?`) | ||
| return value | ||
| }, | ||
| } |
There was a problem hiding this comment.
Why do we need a proxy, if we can just check it here:
https://github.com/jereklas/vitest/blob/bdd74e50836a70d9aae410781f5c6fdbe5093f3a/packages/vitest/src/runtime/mocker.ts#L109
?
Factory can also return a promise that resolves to primitive
There was a problem hiding this comment.
You need the proxy on the exports object because you don't know which export is being accessed until the calling code imports something?
I need to head to bed, but will try taking a look at your factory related concern tomorrow.
There was a problem hiding this comment.
We do need a roxy for exports object, but I don't see why we need proxy for calling a factory - we are calling it ourselves on https://github.com/jereklas/vitest/blob/bdd74e50836a70d9aae410781f5c6fdbe5093f3a/packages/vitest/src/runtime/mocker.ts#L109
There was a problem hiding this comment.
Ah gotcha. I misunderstood your statement.
| const factoryHandler = { | ||
| apply(target: () => unknown) { | ||
| const value = target() | ||
| if (typeof value !== 'object') |
There was a problem hiding this comment.
Should also check null, i think?
|
Looks like a test unrelated to my changes failed in CI? I ran them locally without any issue. What's the process for getting tests re-ran? |
|
Good |
sheremet-va
changed the title
No description provided.