export condition issues with packages using module condition and commonJS modules

[danielroe]

Possible regression from #1401. (Edge releases before then do not exhibit same behaviour.)

This reproduces for lots of Nuxt projects in https://github.com/nuxt/ecosystem-ci, particularly found this one from nuxt-modules/sanity.

Nitro-only reproduction: https://stackblitz.com/edit/github-81ghjr?file=routes/index.ts,package.json,nitro.config.ts

[pi0]

Thanks for reporting with reproduction! This issue seems happening with 2.5.x as well same as repro and also on latest edge. So probably not a regression of #1401 but something we might use to fix it for 2.6.

Checking @sanity/client exports field:

  "exports": {
    ".": {
      "types": "./dist/index.d.ts",
      "browser": {
        "source": "./src/index.browser.ts",
        "import": "./dist/index.browser.js",
        "require": "./dist/index.browser.cjs"
      },
      "react-server": "./dist/index.browser.js",
      "deno": "./dist/index.browser.js",
      "edge": "./dist/index.browser.js",
      "edge-light": "./dist/index.browser.js",
      "worker": "./dist/index.browser.js",
      "source": "./src/index.ts",
      "require": "./dist/index.cjs",
      "node": {
        "module": "./dist/index.js",
        "import": "./dist/index.cjs.js"
      },
      "import": "./dist/index.js",
      "default": "./dist/index.js"
    },
    "./package.json": "./package.json"
  },

It seems more likely that sanity is wrongly using export conditions. "import" and "require" are Standard Node.js conditions and will be used by Node.js in the search path (docs).

Sanity uses "module" condition before "import", itself is asking to prefer using module condition which is not a valid Node.js condition. We use "module" only for backward compatibility. So while we might find some solutions we can do like removing "module" condition support, this is really a simple upstream fix: (well also import + cjs is wrong :D)

{
      "node": {
        "import": "./dist/index.cjs.js",
        "module": "./dist/index.js"
      }
}

---

Oh yeah the other problem is with @sanity/client > get-it. It wrongly uses import condition for @sanity/client/dist/index.cjs and inside it require('get-it/middleware') which causes wrong tracking (esm tracked but actually cjs will be needed!)

[danielroe]

This still seems reproducible on the edge: https://stackblitz.com/edit/github-81ghjr-ta9d1x?file=routes/index.ts,package.json,pnpm-lock.yaml.

[pi0]

For sanity, checking locally it seems nitro is picking exports.import exports.node.module (!) while node is picking exports.node.import

Investigating why

---

update 1: it is rollup + node-resolve behavior somehow

[pi0]

Related issue:

(Context: Discovered while testing nuxt ecosystem failed tests with https://github.com/harlan-zw/nuxt-og-image)

Reproduction: https://stackblitz.com/edit/github-kkcfd3?file=package.json

With #1401, we have added (ESM) export conditions to vercel/nft. When we try to trace files from a CommonJS package like inline-css, node-file trace traces it's sub dependencies using same ESM condition causing issues to properly track dependencies.

[daviddomkar]

Hello. What is the status of this issue? It is quite serious since it has caused me a bug due to which I am forced to pin an older version of the @bufbuild/protobuf package, as discussed here: bufbuild/protobuf-es#610.

I provided a reproduction using nuxt which may be helpful to you: https://github.com/DavidDomkar/bufbuild-protobuf-error-reproduction.

If you clone and do yarn install && yarn build && yarn preview, you can see the webpage with the error on localhost:3000. The problem is, despite the proxy folder being in node_modules of the @bufbuild/protobuf package, specified via "import" export, when running yarn build, the build process creates another node_modules directory in .output/server/node_modules and there the directory is missing, breaking the imports since nitro deletes all other exports except the "module" export. As pointed out by @timostamm in the linked issue.

Can this somehow be resolved?

[pi0]

Hi dear @daviddomkar thanks for making a reproduction and sharing reference.

Checking protobuf-es package file it seems they also have the same export condition ordering issue. I'm afraid nothing much is that we can solve in nitro. If module export condition is earlier, rollup resolver sometimes prefers it (i reopend this issue to be absolutely sure if there is anything we could do, which i think not)

But the fix is pretty easy and straightforward for the package side i will try to follow up in the linked issue. 👍🏼 (bufbuild/protobuf-es#611)

[daviddomkar]

@pi0 Thank you so much for clarifying and giving a concrete solution. As I see here bufbuild/protobuf-es#611 it may not be as straightforward to fix because of other problems inside @bufbuild/protobuf.

[timostamm]

Hey @pi0 👋

We add the "module" export for bundlers, and use an ESM wrapper in "import" for Node.js to avoid the dual package hazard.

I agree that using "module" might not be ideal, and I think we can change our exports as follows:

  "type": "module",
  "exports": {
    ".": {
      "node": {
        "import": "./dist/proxy/index.js" // ESM wrapper specific to Node.js, re-exports CJS
      },
      "require": "./dist/cjs/index.js",
      "default": "./dist/esm/index.js" // pure ESM
    }

With Davids reproduction, yarn run build takes the path node.import, which is also taken by Node.js itself.
We will have to run more extensive tests before we can make this change, but it does look reasonable.

That said, the fact that Nitro is using the "module" condition with the node-server preset really looks like a bug to me. _resolveExportConditions in options.ts returns ['production', 'node', 'import', 'default'] - no "module" in sight, so it might be an issue in Rollup / @rollup/plugin-node-resolve.

Looking at popular npm packages, 44 of them use "module" exports, and most of them prefer it over "import" or "default". Here is a gist analyzing them: https://gist.github.com/timostamm/8f4ba7b08a1cf429c80fac4fa44fa2cd

❤️ the universal features of Nitro, about time for such a project!

[pi0]

Thanks for the through feedback @timostamm. Yes indeed it is a rollup behavior and as far as i could debug, even not directly in node-resolve plugin but more low-level places also use this condition. Really nothing easy to change on this side too.

I think finally we have to hope more packages remove non-standard field. But in the meantime, i might make a auto patch behavior in Nitro to deoptmize tracer when the field is detected. Your idea also seems good for node export condition only see bufbuild/protobuf-es#610 (comment) about other runtimes. bufbuild/protobuf-es#611 is updated for that fix.

[pi0]

The issue with protobuf-es should be solved in the latest version (bufbuild/protobuf-es#645)

I hope module condition won't be used in any more major packages and if they use, they follow same path as sanity and protobuf to at least periorotize it less.

If more serious issues happening, we might opt for manual deoptimization ~> #1988