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

Update package.json #8

Closed
wants to merge 1 commit into from
Closed

Update package.json #8

wants to merge 1 commit into from

Conversation

Asgarrrr
Copy link

No description provided.

@slevithan
Copy link
Owner

Thanks for the PR! I see this addresses #7 filed by @victorteokw.

Can you explain why this is helpful? I would understand wanting to add a require property pointing to a CommonJS module, but since ./dist/index.mjs is an ESM bundle, how does adding a default fallback help?

(Aside: The trailing comma makes this invalid JSON, which needs to be fixed before landing.)

@slevithan
Copy link
Owner

@Asgarrrr @victorteokw Any idea on what behavior this is actually changing and why? I'd be happy to make this change, but would first like to understand why this helps and confirm that it actually solves whatever specific tooling problem you're facing.

@IanMitchell
Copy link

I can't explain the why, but it does look like something in the Vercel pipeline is trying to load this package as cjs.

cache miss, executing 887572bb27302b2a
$ next lint --format json
node:internal/modules/esm/resolve:304
  return new ERR_PACKAGE_PATH_NOT_EXPORTED(
         ^
         
Error: No "exports" main defined in /code/node_modules/oniguruma-to-es/package.json
    at exportsNotFound (node:internal/modules/esm/resolve:304:10)
    at packageExportsResolve (node:internal/modules/esm/resolve:594:13)
    at resolveExports (node:internal/modules/cjs/loader:634:36)
    at Module._findPath (node:internal/modules/cjs/loader:724:31)
    at Module._resolveFilename (node:internal/modules/cjs/loader:1211:27)
    at /code/node_modules/next/dist/server/require-hook.js:55:36
    at Module._load (node:internal/modules/cjs/loader:1051:27)
    at Module.require (node:internal/modules/cjs/loader:1311:19)
    at mod.require (/code/node_modules/next/dist/server/require-hook.js:65:28)
    at require (node:internal/modules/helpers:179:18) {
  code: 'ERR_PACKAGE_PATH_NOT_EXPORTED'
}

Node.js v20.18.0

🤔 the proper fix is probably reaching out to someone on the Turborepo or Next.js team about it

@IanMitchell
Copy link

It appears to happen if you name your config file next.config.ts, and does not trigger if you name it next.config.mjs. So definitely a Next.js issue

@slevithan
Copy link
Owner

slevithan commented Dec 24, 2024

Thanks for sharing that workaround, @IanMitchell. At shikijs/shiki#809 people also mentioned working around the same thing by upgrading to the latest vite or svelte.

My problem with this PR is that, short of a more detailed explanation of what exactly happens that makes this work, it sounds like adding a "default" export makes some pipelines treat the ESM bundle as CJS, which I suspect might cause other issues even if it works in some cases.

Most likely, adding an actual CJS bundle (and a "require" property, instead of "default") would resolve this. But I'm not sure I want to go down that road if there are simple workarounds, since a CJS bundle hasn't actually been requested thus far, and CJS would require long-term support plus changes to how types are exported.

I'm not very knowledgeable about CJS and build pipelines. Very open to feedback/PRs from people who understand this more deeply.

@IanMitchell
Copy link

My guess is this package being listed is a symptom and not the core issue - if you patch something, the next package down the list will throw the same error.

@slevithan
Copy link
Owner

Agreed. Okay, I'm going to close this PR since it seems like it's not the right fix. But hopefully the workarounds described here are helpful for anyone encountering the same issue.

@slevithan slevithan closed this Dec 24, 2024
@msiric
Copy link

msiric commented Dec 24, 2024

My file is named next.config.mjs and I'm still getting the same error.

@slevithan
Copy link
Owner

@msiric Are you using Vite, and if so have you upgraded to 5.x? Other people in the issue I linked fixed it it by upgrading Vite or Svelte. I'm guessing that some build tool that is a common dependency of them is the source of the problem, and needs to be updated to a newer version.

@msiric
Copy link

msiric commented Dec 24, 2024

@slevithan I'm using Webpack with my Next.js app. No Svelte or Vite.

@wereHamster
Copy link

FWIW, Node.js recommends including "default":

When using environment branches, always include a "default" condition where possible. Providing a "default" condition ensures that any unknown JS environments are able to use this universal implementation, which helps avoid these JS environments from having to pretend to be existing environments in order to support packages with conditional exports. For this reason, using "node" and "default" condition branches is usually preferable to using "node" and "browser" condition branches.

Source: https://nodejs.org/api/packages.html#conditional-exports

@slevithan
Copy link
Owner

Good insight, @wereHamster. However, the "environment branches" that quote refers to are things like "node-addons", "node", and "browser". oniguruma-to-es is not using environment branches in its conditional exports, so that doesn't apply in this case.

I'd be fine with accepting this PR if I understood (or someone could explain) why it helps avoid the issue a few people are facing, so I could have more confidence that adding "default" in addition to "import" (despite only offering an ESM bundle) wouldn't break things or lead to harder to debug issues for other setups that are incorrectly trying to load oniguruma-to-es as CJS.

Alternatively, if someone could provide a simple/reduced demo repo that is easy to clone for testing and reproduces the problem, I could try to play around with other package.json properties that might work around the issue.

However, as @IanMitchell has pointed out, the error a few people are seeing when loading this package is almost certainly a symptom and not the cause of whatever the underlying issue is in some build pipelines.

@victorteokw
Copy link

People who use next.js with ts config, when Shiki is used for code coloring, it requires this package to have a "default".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants