-
-
Notifications
You must be signed in to change notification settings - Fork 31.7k
mock.module: consolidate defaultExport
+ namedExports
into exports
#58443
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
Comments
defaultExport
+ namedExports
into exports
defaultExport
+ namedExports
into exports
CommonJS? EDIT: Actually, I guess CommonJS can be handled by this change. |
After thinking about this more this seems like it might be a more confusing user experience (I might be misunderstanding how the proposed changes will work). If I provide the example mock exports: exports: {
default: mock__default,
foo: mock__foo,
}, Don't I now need to know more about the module that I'm mocking - specifically if it is CJS or ESM? If I'm mocking ESM, then I get a default export and a named export. However, if I'm mocking CJS, then I get two named exports, with one of them being named
Another note here is that quibble, for example, does separate the named and default exports. Jest is also not known for its great support for both module systems. |
Ahh, interesting. Buut I don't see how this could be possible: {
defaultExport: mock__default,
namedExports: { foo: mock__foo },
} In CJS, wouldn't import someCjs from 'some-cjs';
// 💥 import { foo } from 'some-cjs';
typeof someCjs; // 'function'
typeof someCjs.foo; // 'function' (I'm not sure what happens via require here: And if the mocked module was ESM: import someEsm, { foo } from 'some-esm';
typeof someEsm; // 'function'
typeof foo; // 'function'
typeof someEsm.foo; // 'undefined' |
Good point. Yes, that's how it works. I don't think you can really avoid it in CJS. We could do the same thing in ESM under the hood, but I don't think that would be a good idea. I still think the current behavior is significantly less confusing than my example in #58443 (comment). Currently, when the mock is created, This has also been the behavior since the API was introduced. We shouldn't break existing users just because an API is experimental unless there is a good reason to do so - we saw how bothered people got with the loader hooks changes. I also don't think creating a automated migration and changing the API in v25 would be adequate because there wouldn't be a consistent API across all of the supported versions. I think you would need to wait until there was a single usable API on all supported release lines before removing things. I guess I don't see any tangible upside to this proposed change, but I do see disadvantages. |
Actually, I was wrong: it can be done to work the same: // foo.cjs
module.exports = function foo() {}
module.exports.a = 'a';
// does not work via `Object.assign(module.exports)` // main.mjs
import foo, { a } from './foo.cjs';
console.log({ foo, a });
I believe this can be applied to both options shapes (
One impetus is facilitating adoption of node's test runner. Another is ergonomics (I think this is an ergonomic improvement—no shade to whoever chose the current). If we provide an automated migration, the burden to users seems trivia; for me as a user, I would happily run a quick script once to improve my DX going forward (I think we should include info about the migration in the deprecation message). I would also be okay leaving the separated ones for longer. I wish we had a way to track usage so we know can judge user-preference (but that's another can of worms). BUT we can know how many times the migration is used, which could give us an idea.
Touché 😅 This is significantly smaller/less complex and (IMHO) far less controversial, especially if we provide a migration. We can also do a better job this time advertising the incoming change. |
If we can make both APIs work the same, I don't care too much about the actual format of the options object. I do care about breaking users though. I think an alternative plan could look like this:
As a side note, I think always trying to do whatever Jest does in general should be a non-goal. |
That sounds good to me!
I don't either 😉 In this case, I think they got it right though. |
Currently,
mock.module
'soptions
expectsdefaultExport
andnamedExports
separately. We are not aware of a reason to separate them, and it's inconsistent with other major testing frameworks/utilities (such as Jest).The plan would be:
options.exports
defaultExport
&namedExports
intooptions.exports
defaultExport
&namedExports
deprecateddefaultExport
&namedExports
The text was updated successfully, but these errors were encountered: