interopDefault proxy issue with testing

[lachieh]

Environment

See reproduction.

Reproduction

https://stackblitz.com/edit/jiti-interopdefault-testing?file=src%2Fmain.ts

Describe the bug

In upgrading to v2 for docusaurus, I'm discovering some odd behavior.

It seems that the Proxy implementation for interopDefault may not be entirely working as expected. In the repro link, you can see that the properties are accessible, but only when directly accessing the properties (since that's how Proxies work). The issue is that this is seemingly causing issues with testing.

Additional context

We are tracking the upgrade at facebook/docusaurus#10716

Logs

No response

[pi0]

Thanks for explaining the issue. I understand that the new behavior affects testing when modules are strictly being compared as objects.

Unfortunately, it is not easy to avoid proxy, as it covers many edge cases in mixed CJS/ESM modules.

I'm wondering is it affecting any real usecase in docusaurus too with wrong behavior of proxy?

[lachieh]

Yeah, I'm investigating that now. It seems like it does since the E2E tests were failing, specifically when importing the main config file in esm format. I actually just ended up wrapping the imported proxy in a mlly.interopDefault call and it solved 99% of the test failures but that feels a little strange.

The last remaining test failure seems to be something to do with a nested dependency not returning the proxy object. The usage of jiti is here. If you can see anything that might cause the dependencies to skip the jiti import, I'm all ears.

[pi0]

Are both modules and configs having a default export? In this case, you might want to use jiti.import(id, { default: true }) (we use it internally for c12, config loader of Nuxt and Nitro)

[lachieh]

No, plugins use named and default exports, and configs (typically) have a default export. Changing this behavior would be breaking so wouldn't be able to happen until the next major, and I'm not sure that is planned in the near future.

For more detail, the larger test logic looks like this:

1. Test runs, creating the entryFile, and some dependency files with the fileHelper function:
   https://github.com/facebook/docusaurus/blob/6aa92ae28dda326bee6ec0de1ed493e068b022ca/packages/docusaurus-utils/src/__tests__/moduleUtils.test.ts#L118-L131
2. entryFile.load() calls the loadFreshModule function which returns the jiti.import()'ed module.
   https://github.com/facebook/docusaurus/blob/6aa92ae28dda326bee6ec0de1ed493e068b022ca/packages/docusaurus-utils/src/__tests__/moduleUtils.test.ts#L25-L34
3. the resulting import can see the named exports on the entryFile, but not on the imported dependencies.
   https://github.com/facebook/docusaurus/blob/bd88110d417ef357a592ebc218680302abb4798d/packages/docusaurus-utils/src/__tests__/moduleUtils.test.ts#L134-L143

[lachieh]

I will say that everything works as expected on 2.1.2 (last version before the proxy change, I believe), except for the need to add the --experimental-vm-modules flag, which was fixed in 2.4.0.

Would you consider a PR that made the interopDefault option into bool | (sourceModule: any, opts?: {...}) => any so that I can provide the mlly.interopDefault function to skip the Proxy implementation? This is probably more of a hack than a long term solution.

[pi0]

Surely feel free to make a PR to make interopDefault programmatically configurable.

Consider that interopDefault is used in all intermediate modules. mlly.interopDefault has known issues and alone is not a safe replacement in jiti v2.

---

I have another proposal. We could add a symbol to the jiti's interop proxy that allows accessing the raw module value to manually mix (but only for top level export). Currently something like this is also possible and I think should have similar effect (except that has one additional default)

const { default: _default, ...namedExports } = entryWithDeps // proposal: entryWithDeps[Symbol.for('jiti.raw')]
const mixedExports = { ..._default, ...entryWithDeps }

[lachieh]

I like that second option much better. Would you want another option in jiti.import() like default? Perhaps jiti.import('./file', { raw: true })?

[pi0]

We can do that too! Or what about default: false that skips interop default for final exports result? (currently it is true or nothing)

[lachieh]

On second thought, adding an option means it's potentially "normal" to access the module that way, and default: false might be confusing for newcomers just looking at the function call.

There are footguns with the mixed approach (see below), so while it will be nice to have the same functionality as v1, it seems like behavior that might want to be marked as unsafe and deprecated/removed in the next major version.

// file1.ts
export const named = 42;

export default {
  named: 43
}

// file2.ts
const mod = require('./file1.ts');
console.log(mod.named) // should this be 42 or 43?

Perhaps the symbol is exported as such?

// inside 'jiti' module
export const UNSAFE_FLAT_MODULE_EXPORTS = Symbol.for('jiti.unsafe_raw')

// user code
const entry = await jiti.import('./entry.ts');
const flatModule = entry[UNSAFE_FLAT_MODULE_EXPORTS];

[pi0]

Symbol.for('jiti.raw') can be directly used in docusaurus to access raw module. It is not unsafe just not interop version of module namespace. I agree might be better to keep it mainly as a workaround (and later if common enough we can think of a flag)