fix: check exactly in proxyESM

[zhangyuang]

Description

The below code proxyESM function judge is not exact in some case like vuex see this pr which not have named export in cjs file. vite use resolve.sync get main field from module package.json which point at a commonjs format file generally

Additional context

function proxyESM(id: string, mod: any) {
  const defaultExport = mod.__esModule
    ? mod.default
    : mod.default
    ? mod.default
    : mod
  return new Proxy(mod, {
    get(mod, prop) {
      if (prop === 'default') return defaultExport
      return mod[prop]
    }
  })
}

when import { createStore } from 'vuex', resolve.sync will return vuex.cjs.js file and this is vuex export code

It cause we can't use named import from vuex only get undefined

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[zhangyuang]

cc @Shinigami92 @patak-js thanks 😊

[zhangyuang]

I will add test as soon as possible

[zhangyuang]

test has done, please view it @Shinigami92 😄

[Shinigami92]

Seems it was good that we add tests 🙂

[zhangyuang]

Is there any progress?

[zhangyuang]

has updated 😊 @Niputi

[zhangyuang]

PTAL @sodatea 😊

[sodatea]

Though this PR does solve the issue for vuex, I feel it's more like a hack than a fix.

Let's fix it in vuex instead. vuejs/vuex#2081

[zhangyuang]

@sodatea yeah,but not only vuex many third-party package in npm has the same error,maybe we can fix it by vite？like fs-extra

[sodatea]

Oh, I get it.

There are several cases here:

1. For packages that have ESM exports like vuex, the root issue is Vite doesn't support ESM packages with "exports" field in SSR #3953
   1. it can be fixed by fixing Vite doesn't support ESM packages with "exports" field in SSR #3953
   2. or by aligning the CJS exports with ESM exports as in feat: add named export in cjs format file vuejs/vuex#2081
2. For pure CJS packages like fs-extra, the problem is that the change proposed in fix: add import support to ssrModuleLoader #5197 wasn't fully CJS-compatible.
   • The assumption in that PR In node@12+ we can use dynamic import to load CJS and ESM didn't cover non-static-analyzable CJS modules.
   • If a certain export (say, var foo = 'foo'; exports[foo] = ) failed to be detected by cjs-module-lexer (because foo is a variable, not a string literal), when imported, it would go into the default export instead of becoming a named top-level export.
   • But when required (the behavior before fix: add import support to ssrModuleLoader #5197), exports[foo] is accessible as a top-level export.

So yeah, there needs to be a compatibility layer in proxyESM.

Thanks for bringing this up!

[aleclarson]

This approach is fragile. If a named export equals undefined or null, the default export is assumed to be an object, but we can't guarantee that.

#5197 was using this logic in proxyESM...

if (prop in mod) {
  return mod[prop]
}
// commonjs interop: module whose exports are not statically analyzable
if (isObject(mod.default) && prop in mod.default) {
  return mod.default[prop]
}
// throw an error like ESM import does
throw new SyntaxError(
  `The requested module '${id}' does not provide an export named '${prop.toString()}'`
)

...until @matthewp reverted it without explanation.

[patak-dev]

Thanks for digging into this, @natemoo-re @matthewp could you check why this was removed? Maybe as part of the trials to make CI pass? Let's add this back before releasing 2.7

[natemoo-re]

I'm not sure why this was removed, but this approach LGTM.

[matthewp]

The throw was a problem there. Since mod is an object, any code that imports the namespace can inspect it for keys just like you can do with any object. The throw assumed that the get always comes from a static import statement but that's not necessarily the case:

// This is valid code
import * as mod from './something.js';

if(mod.someKeyThatDoesntExist) {
  // Oops this threw
}

Also the second condition looks wrong to me too. Given this example:

mod.js

export default { color: 'green' };

main.js

import * as mod from './mod.js';

console.log(mod.color);

Should log undefined but will log 'green' instead. If commonjs interop is needed it needs to verify that the module is commonjs first.