fix: reverse node export conditions

[danielroe]

resolves #308

this tries to fix an issue we're experiencing triggered by the module export condition being added. Because the first matching condition is picked, it would likely be safer to add the import before module bundler.

(I would also suggest removing the module condition as it's not supported by node, and this is within the node condition. But I'm not sure why it was added in the first place so I'm reluctant to make changes.)

[pi0]

I agree with changes of module ordering 👍🏼 But worth to note that this is still not a valid condition as import field is pointing to a CommonJS entry. Using an ESM stub + require field could be nice (internally using createModule to reexport)

[danielroe]

confusingly, despite the name, it's not a CJS entry:

https://unpkg.com/browse/@sanity/client@6.4.9/dist/index.cjs.js

[stipsan]

Bundlers like Webpack in Next.js target the export condition pattern we're using. We specifically need only nodejs to use the node.import condition because it's the only environment that comprehends the ESM CJS re-export pattern. When bundlers like those in Astro encounter this pattern, they fail. In Next.js, it also bloats the bundle due to added cruft from the re-export and module format change. node.module is a condition ignored by nodejs but targeted by bundlers like webpack. It signals transpilation expectation for bundlers. We use both patterns to avoid issues in native nodejs environments, especially since some dependencies in @sanity/client aren't node-esm-ready.

Regarding your error, it seems to be trying to use node.import rather than node.module. So, removing node.module might not solve the issue.

[danielroe]

The error message in the linked run is a little confusing. We use http://github.com/vercel/nft in Nuxt and Nitro to trace dependencies, and it is picking the 'module' condition and only copying the module across, but of course node is looking for the 'import' condition and this file is missing.

I have tested and confirmed this fix works for us (as does removing the 'module' condition entirely) but of course it does need to work for the rest of the ecosystem too. Do you have a Next.js project I can test with to confirm?

[stipsan]

We have a matrix you can use. If your nft setup is working optimally and supports the 'module' condition then it shouldn't be hitting the 'node.import' one. I fear the changes in this PR will cause the problem in Astro to regress 😅
Another case that's related and why we've started using node.module in this way: portabletext/react-portabletext#70

[stipsan]

We would of course prefer to be able to drop the exports.node condition entirely, but previous attempts have failed 😅

[danielroe]

Thanks, that's helpful. I'll investigate and see if I can find a fix which works with the matrix. FWIW, we don't want the module dependency. We want the import one to be picked as we output native node ESM, which is why I initially suggested moving module below it.

[stipsan]

But you're also using a bundler, which the node.module one is for, as it's native ESM. The node.import one is for non-bundlers that solves the problem of node sometimes misbehaving when it's in its native ESM runtime, and no bundler is safe-guarding and ensuring that dependencies of our dependencies aren't fake-ESM that only works if a bundler is transforming it 😅
You don't want node.import, you want import or node.module in this case is what I'm arguing.
What if we move node.module to module, would that solve your case? That shouldn't break cases like Astro

[danielroe]

No, we're not using a bundler in this case. We're copying across files to a new tree-shaken node_modules folder. So we need to copy across only the import condition file as that's what Node expects to be present when we run our server with an external @sanity/client.

[stipsan]

Copying files without a bundler! I see, then copying ./dist/index.cjs.js and ./dist/index.cjs is what you need for node 👍

[danielroe]

Exactly! Check out https://github.com/vercel/nft - it's pretty cool.

[stipsan]

Yeah, did an internal search and in one place we're using nft we set these options to:

  conditions: ['module', 'node']

Maybe try that?

[stipsan]

There's precedence for it but not a lot of information out there. In fact, there's not a lot of information on how to ship ESM and CJS, or JS that works universally with the current npm ecostystem, is there 😅

[pi0]

If I may weigh in, we have been supporting ESM modules for nuxt since 2018 alongside webpack 4 and early versions. Back in time, there were no concrete standards for ESM modules, and there was a top-level module filed unofficially introduced by webpack to help the situation and pick a bundler-friendly format (meaning mixed CJS/ESM allowed in this one!)

Once Node.js tried to make exports field a standard, they avoided that top-level module field (and module export condition) for the very intentional reason to avoid confusing with previous module formats.

I wish it could be called module condition but it is not and Node.js only applies resolution with import format for importing a module. And at the same time, lots of toolings like Rollup, (sadly) try to apply module condition for resolution even when dependencies like @sanity/client end up being an external (and as a valid node external, expected to be resolved with import condition).

Solving previous mistakes is not easy and bundlers like rollup can easily not apply module condition but I think it would be really better if packages like @sanity/client prefer and respect the most recent standards and use import, require as the first two conditions.

This way, the @sanity/client still remains a valid Node.js package, stays compatible with framework bundler configurations that want to prefer (non-standard) module field but also works with rollup externals allowing import condition to work.

I hope the above context will be helpful for finally making your decision.

[stipsan]

Hi @pi0 @danielroe,

Thanks for your feedback. Our initial need for node.module was for Astro compatibility. However, with Astro v3, this issue seems resolved.

While I value the bundlesize savings in frameworks like Next.js, I'll prioritize ecosystem compatibility. If I don't find a workaround by EOD, I'll remove node.module. Thanks for your understanding! 🙌