Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ESM isn't published correctly #127

Closed
dburles opened this issue Nov 2, 2022 · 7 comments · Fixed by #133
Closed

ESM isn't published correctly #127

dburles opened this issue Nov 2, 2022 · 7 comments · Fixed by #133
Labels
bug Something isn't working released

Comments

@dburles
Copy link

dburles commented Nov 2, 2022

As there's no type field set in package.json, Node will assume anything with a .js file extension to be CommonJS and treats the exported ESM as CommonJS.

Reproduction:
> npm i next-sanity
> echo 'import { groq } from "next-sanity";' > test.mjs
node test.mjs

Results in:

import { groq } from "next-sanity";
         ^^^^
SyntaxError: Named export 'groq' not found. The requested module 'next-sanity' is a CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'next-sanity';
const { groq } = pkg;

Solution:

  1. Rename all ESM exports to .mjs, and CommonJS exports to .cjs (Recommended).
    or
  2. Set package.json type as "module" (so Node then treats .js as ESM) and then rename all extensions of commonjs exports to .cjs.

Also, if the package is to remain publishing dual bundles, you need to ensure that there's no dual package hazard.

For more information see the Node.js docs: https://nodejs.org/api/packages.html#dual-commonjses-module-packages

@github-actions
Copy link
Contributor

github-actions bot commented Nov 3, 2022

🎉 This issue has been resolved in version 1.0.5 🎉

The release is available on:

Your semantic-release bot 📦🚀

@stipsan stipsan added the bug Something isn't working label Nov 3, 2022
@stipsan
Copy link
Member

stipsan commented Nov 3, 2022

Thanks for your report @dburles! We don't have a dual package hazard concern in this package, so we opted for 2. 😄

@dburles
Copy link
Author

dburles commented Nov 3, 2022

Looks great, thanks!

@pm0u
Copy link

pm0u commented Dec 9, 2022

I am receiving this error in Typescript when attempting to import the client:

// lib/sanity/index.ts
import { createClient } from 'next-sanity'
...
$ tsc --noEmit --strict --incremental false

lib/sanity/index.ts:1:30 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("next-sanity")' call instead.
  To convert this file to an ECMAScript module, change its file extension to '.mts', or add the field `"type": "module"` to '***/package.json'.

1 import { createClient } from "next-sanity"
                               ~~~~~~~~~~~~~


Found 1 error in lib/sanity/index.ts:1

This is on next-sanity@3.1.2

It seems the issue stems from moduleResolution: "Node16" in the tsconfig.json, however we need this to leverage the "exports" field in package.json for other imports. We have the "esModuleInterop" set to true in our tsconfig.json and I also tried "allowSyntheticDefaultImports" to no avail

Edit: I'm going to leave this here, but it seems the larger issue is the strange behavior with setting "Node16" as the resolution mode. This thread is a bit over my head: microsoft/TypeScript#49083 but it seems this particular method of module resolution is not to be used frivolously. We are going to work around the reasons we needed that import method as it likely implies issues with the modules that needed it.

Followup again: I don't know if we can actually refactor the places that require "Node16" module resolution at this time. This is used to leverage the "exports" field of package.json in some internal packages. This could be an error on our end.

I can confirm at least for our use case, just slapping a // @ts-expect-error above the import allows it to work fine with "moduleResolution": "Node16" in tsconfig.json... so, not sure what the issue is exactly. we are going to try this out for now since continuing to us the Node16 resolution is a lot simpler in other areas of our codebase currently.

@stipsan stipsan reopened this Dec 9, 2022
@pm0u
Copy link

pm0u commented Dec 16, 2022

To further add to the confusion, this error is apparently not there in our netlify deploy which causes it to fail since I used a //@ts-expect-error:

4:25:17 PM: $ yarn build
4:25:17 PM: yarn run v1.22.19
4:25:17 PM: $ next build
4:25:17 PM: info  - Linting and checking validity of types...
4:25:19 PM: Failed to compile.
4:25:19 PM: 
4:25:19 PM: ./lib/sanity/index.ts:2:1
4:25:19 PM: Type error: Unused '@ts-expect-error' directive.
4:25:19 PM: > 2 | // @ts-expect-error
4:25:19 PM:     | ^
4:25:19 PM:   3 | import { createClient } from "next-sanity"
4:25:19 PM:   4 | import createImageUrlBuilder from "@sanity/image-url"
4:25:19 PM:   5 | import { config } from "./config"

unsure if this is some dependency difference or what.. I don't know how to run the same command that NextJS is outside of the build command

@pm0u
Copy link

pm0u commented Dec 17, 2022

I think this is still an issue with this library, I believe the reason it seemed to "resolve" was because when NextJS builds it apparently forces "moduleResolution": "node" according to the issue I referenced in the one below.

vercel/next.js#44095

@stipsan
Copy link
Member

stipsan commented Jan 8, 2023

This problem should now be solved in v4: https://github.com/sanity-io/next-sanity/releases/tag/v4.0.0 🙇

@stipsan stipsan closed this as completed Jan 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants