fix(esm): support nuxt-module-build

[stipsan]

Fixes #308. Fixes #309.

Tested by checking out @nuxtjs/sanity and running these commands on main:

pnpm install @sanity/client@canary --save-dev -w && pnpm build && pnpm test -- --coverage

Output:

      Coverage enabled with v8

     ⠋ [ beforeAll ]
 ✓ test/unit/client.test.ts (10)
 ✓ test/unit/client.test.ts (10)
 ✓ test/unit/helpers.test.ts (3)
 ✓ test/e2e/ssr.test.ts (5) 7487ms
 ✓ test/unit/sanity-image.test.ts (31)
 ✓ test/unit/sanity-file.test.ts (2)
 ✓ test/e2e/ssr-minimal.test.ts (5) 6366ms
 ✓ test/unit/sanity-content.test.ts (14)

 Test Files  7 passed (7)
      Tests  70 passed (70)
   Start at  19:59:44
   Duration  8.19s (transform 287ms, setup 0ms, collect 1.10s, tests 13.96s, environment 736ms, prepare 549ms)

 % Coverage report from v8
------------------------|---------|----------|---------|---------|-------------------------------------
File                    | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s                   
------------------------|---------|----------|---------|---------|-------------------------------------
All files               |   93.41 |    86.75 |   86.04 |   93.41 |                                     
 playground             |     100 |      100 |     100 |     100 |                                     
  nuxt.config.js        |     100 |      100 |     100 |     100 |                                     
 src/runtime            |   79.25 |       80 |   58.33 |   79.25 |                                     
  client.ts             |     100 |      100 |     100 |     100 |                                     
  composables.ts        |   64.63 |    33.33 |   33.33 |   64.63 | 27-29,36-37,45-46,52-55,63-71,74-82 
  groq.ts               |   41.17 |        0 |       0 |   41.17 | 8-17                                
 src/runtime/components |   98.43 |     87.9 |   96.77 |   98.43 |                                     
  sanity-content.ts     |   97.39 |     87.5 |   94.44 |   97.39 | 180-181,212-214,258,263             
  sanity-file.ts        |     100 |    66.66 |     100 |     100 | 36-41                               
  sanity-image.ts       |   99.49 |    93.33 |     100 |   99.49 | 195                                 
------------------------|---------|----------|---------|---------|-------------------------------------

@danielroe @pi0 if you can confirm the fix is working on the canary release on your end we'll merge and release promptly 🙌

[stipsan]

cc @rexxars since we have "type": "module" declared it's enough to point to the ESM in default, no need to have both import and default.

The module statement here is ignored in most environments and bundlers. Except for in rollup and webpack, which gives it the same precedence as it got when it lives at node.module.
I'll run a double check to be sure and come back but so far all tests are green with this change.

[pi0]

since (and as long as) value of module and default are the same, it should be fine. But i am worried if module points to another entity in the future, same issue will pop out as rollup (which is used by nitro/nuxt) will pick it and it is really not easy to disable this behavior in rollup.

I guess also since module and default are same, it is unnecessary to keep module condition (any implementation of exports resolver, should fallback to default, which is the same)

[stipsan]

@pi0, we utilize module to deter webpack from defaulting to node.import as outlined here. It's true this also affects rollup. Additionally, our @sanity/pkg-utils tool, built on rollup, requires module to align with either default or import when set to `type: "module", ensuring consistent future references.

The primary role of exports.module is directing bundlers away from node.import to module, eliminating the need for re-export wrappers. Our eventual aim is to phase out instanceof checks, guaranteeing a smooth runtime even with mixed CJS and ESM @sanity/client instances. This will enable us to streamline our exports, enhance compatibility, and hopefully resolve related issues.

[stipsan]

After some more end-to-end testing it turns out moving module below node negates it so it's not preferred after all. Updated the PR to just remove it as originally suggested.
Made a follow-up ticket to update @sanity/pkg-utils to stop recommending the node.module pattern and instead focus efforts on refactoring out of needing to protect against dual package hazards 🙌

[pi0]

Much better and appreciated, thanks! (BTW do you think require makes sense outside of Node condition? 🤔)

Unrelated: You might want to use types + default for new Node.js type resolution. You can validate with https://arethetypeswrong.github.io/

[stipsan]

(BTW do you think require makes sense outside of Node condition? 🤔)

Maybe not, but let's say these two variants are functionally identical:

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

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

And in both cases only ever node is loading up ./dist/index.cjs if require('@sanity/client') happens and it's likely ignored by all other bundlers.
If that turns out to be the case I still feel it's not worth the risk of someone's bundler setup breaking, since we don't gain much by making this change 😅

[stipsan]

Regarding the arethetypeswrong tip thnx, made a note to follow up in @sanity/pkg-utils to make the changes necessary to ship typings that report the correct module formats used to TypeScript.
I have to admit we haven't started using the new "moduleResolution": "node16" or similar in our own repos. We're using either the less strict "moduleResolution": "bundler" introduced by typescript v5, or good old "moduleResolution": "node" 🙌

[pi0]

I haven't tested but it might affect Bun (and Deno's compatibility) to prefer CJS over ESM. Let's try this way 👍🏼

[danielroe]

all good in my testing 👌

thank you ❤️