Open GeoffreyBooth opened 4 days ago
cjihrig
commented
4 days ago I think the simplest way to solve this would be to use the hint if present. If the hint is not present, use some default. To be more powerful/flexible, we could add another option to the API that lets the user be explicit about the format they want.
GeoffreyBooth
commented
4 days ago I think the simplest way to solve this would be to use the hint if present. If the hint is not present, use some default. To be more powerful/flexible, we could add another option to the API that lets the user be explicit about the format they want.
What does this mean in practice? If resolve returns format use it, and if not, use module if the user has defined both defaultExport and namedExport; or use commonjs otherwise?
I think rather than having the docs explain how the hint is determined based on URL and when it might not exist and what happens in that case, et cetera, a simpler approach could be explained in the docs as follows:
When only one of
defaultExportornamedExportsis defined, the mock will be created as a CommonJS module so that it can be used via eitherrequireorimport. If bothdefaultExportandnamedExportsare defined, the mock will be created as an ES module that can be accessed viaimport, and also viarequireif--experiemental-require-moduleis passed.
cjihrig
commented
3 days ago use module if the user has defined both defaultExport and namedExport
I would avoid using this heuristic because defaultExport and namedExports are valid and useful for many CJS use cases.
How many cases in the current module mocking tests begin to fail with the changes in #53619? If it is a lot, then I think we may want to figure out an appropriate default (maybe 'commonjs' as suggested in https://github.com/nodejs/node/pull/53619#issuecomment-2197133656). If it is one or two cases, then maybe we keep the default behavior of throwing when there is no known format and just require users to pass an option in those cases.
GeoffreyBooth
commented
3 days ago How many cases in the current module mocking tests begin to fail with the changes in https://github.com/nodejs/node/pull/53619?
In ./node --experimental-test-module-mocks --experimental-require-module ./test/parallel/test-runner-module-mocking.js the following fail:
Basically any test where the mock URL is referring to a .js file where there’s no package.json with a type field, since for all these cases the format is undefined and the mocking code has an assertion that format be one of builtin, commonjs or module.
Since there’s no file at the URL provided, I’m not sure why we’re using anything about the URL as part of determining format. That part feels wrong to me.
I would avoid using this heuristic because defaultExport and namedExports are valid and useful for many CJS use cases.
Because of the time when they can be used together, if the default export happens to be an object with properties that match the provided named exports?
It seems to me like the most convenient API would be one where the sources are always generated as ESM and --experimental-require-module is enabled automatically when this API is used, at least for requiring these mocks. If that feels too risky, then sure, asking the user to specify the format is better than guessing based on a URL of a file that doesn’t exist.
GeoffreyBooth
commented
3 days ago Actually I might’ve been mistaken about one part: does the URL of the module to be mocked need to exist? Because if we only support mocking modules that exist, we can use nextLoad to get the format, and then the API doesn’t need to change. It’s only if we support mocking URLs of nonexistent modules that we need some new way of determining the format.
It seems like @cjihrig above is referring to this? https://nodejs.org/docs/latest/api/test.html#mockmodulespecifier-options. Which seems to be implemented here? https://github.com/nodejs/node/blob/02bd8662985a2c4e11f3235f3fc5bb28b801c3c4/lib/internal/test_runner/mock/mock.js#L489-L516
Per the docs,
mock.modulesupports defining mocks for both module systems based on passed-in objects (not source code). Then we generate source code here? https://github.com/nodejs/node/blob/02bd8662985a2c4e11f3235f3fc5bb28b801c3c4/lib/test/mock_loader.js#L163-L175At first I was confused as to how this works, since ESM is unlike CommonJS in that ESM can have a default export that’s something other than an object whose properties are the named exports; so how can this one API generate mocks that apply to both module systems while still supporting this ESM-only feature. But then I noticed that an exception gets thrown if the mock is created with both
defaultExportandnamedExportsdefined and the module is required. So I guess really it supports both module systems only if you give it input that can be consumed by both systems.But maybe that’s the answer: if the user defines one of
defaultExportornamedExports, it creates the source as CommonJS, and either maps the provideddefaultExporttomodule.exportsor it adds each of thenamedExportsontomodule.exports. If the user defines bothdefaultExportandnamedExports, the mock is created as ESM and the restrictions mentioned in the docs apply.Alternatively the mock could always be created as ESM, though then you have the issue that for it to be
require-able--experimental-require-moduleneeds to be enabled. But maybe that behavior could be opted into by using this API. Since the user is passing in an object I assume we don’t need to worry about top-levelawait.Or we could just ask the user what format to generate the source code in. We kind of already are, though the users probably don’t realize it, since we ask for the URL of the module to be mocked and base the format on that. Though it seems like you’re trying to keep this as simple as possible so if there’s a way to avoid adding another option, I assume you’d prefer to determine this implicitly. I think determining format based on
defaultExport/namedExportsis a better source to use rather than how the file URL resolves.