leafac / rehype-shiki

Rehype plugin to highlight code blocks with Shiki
MIT License
31 stars 3 forks source link

Vercel Build Error #7

Closed OskarGroth closed 2 years ago

OskarGroth commented 2 years ago

Looks to be the same problem as https://github.com/leafac/rehype-shiki/issues/4.

I'm getting this error even though my project is ESM.

Error [ERR_UNKNOWN_BUILTIN_MODULE]: No such built-in module: node:assert/strict
--
18:33:34.007 | at new NodeError (internal/errors.js:322:7)
18:33:34.007 | at Loader.builtinStrategy (internal/modules/esm/translators.js:285:11)
18:33:34.008 | at new ModuleJob (internal/modules/esm/module_job.js:63:26)
18:33:34.008 | at Loader.getModuleJob (internal/modules/esm/loader.js:258:11)
18:33:34.009 | at async ModuleWrap.<anonymous> (internal/modules/esm/module_job.js:78:21)
18:33:34.009 | at async Promise.all (index 2)
18:33:34.009 | at async link (internal/modules/esm/module_job.js:83:9) {

Related:

https://github.com/vercel/next.js/issues/28774

node:assert/strict

This is originating from the use of node:assert/strict here: https://github.com/leafac/html/blob/28b93c91e532a4b5766bff7ad84ab26a412db7e4/source/index.ts

I noticed @re-taro was able to resolve this in a fork here:

https://github.com/re-taro/html/blob/main/source/index.ts

Since node:assert/strict is breaking the use of this whole lib in production right now, do you think you could remove it in the html package altogether since it only seems to be used for a simple debug test?

leafac commented 2 years ago

Hi @OskarGroth,

Thanks for reaching out and for the detailed report.

Sure, let’s do something to this import to fix things for y’all. I want to understand the issue a little better to determine what will be the best solution:

  1. What part of the ecosystem lacks support for the node: protocol? Next.js? Vercel? A module bundler used by one of them?

  2. Why doesn’t this part of the ecosystem support the node: protocol?

  3. Wouldn’t it make more sense to add this feature on their project, instead of trying to adapt packages around the limitation? (Sure, in html’s case the import is just for testing purposes, but in general this will affect anything using a Node.js standard library, which is pretty much anything useful.)

  4. If we ignore the good practice of marking imports of Node.js’s standard libraries with the node: protocol, then does it work? For example, if instead of import assert from "node:assert/strict"; we had import assert from "assert/strict";?

Thanks for helping out!

OskarGroth commented 2 years ago

From what I understand:

Next.js adds Webpack configuration fallbacks for node built-in modules by using Webpack config option resolve.fallback. This feature does not support node: prefix. Just a couple of specific modules is included in this fallback, assert happens to be one of them. The full list: https://github.com/vercel/next.js/blob/384953b35c5e9935bb4a2fcdfe5056efb73cd740/packages/next/build/webpack-config.ts#L628-L659

As a result, including a module that imports node:assert from a NextJS project will fail to build on Vercel (I'm on latest NextJS).

Not sure if this originates from NextJS or Webpack or both. There's many similar issues across both libs.

See: https://github.com/vercel/next.js/issues/28774#issuecomment-1088330447 https://github.com/webpack/webpack/issues/14166

Even if fixed soon (unlikely), it would mean this library would remain unusable for anyone who isn't running cutting-edge version of NextJS. So while I agree it's not responsibility to fix this, adjusting a bit to work around this issue would enable NextJS users to use your library.

If we ignore the good practice of marking imports of Node.js’s standard libraries with the node: protocol, then does it work? For example, if instead of import assert from "node:assert/strict"; we had import assert from "assert/strict";

It is my understanding that this would also resolve the issue, but I'm not sure.

Thanks!

leafac commented 2 years ago

@OskarGroth: Your point of view is perfectly reasonable. Can you please test changing from import assert from "node:assert/strict"; to import assert from "assert/strict"; and see if that works with your project? A quick-and-dirty way to test it would be editing the file under node_modules/rehype-shiki/. If it works, it may be the easiest workaround.

Other than that, I think it makes sense to help NextJS, webpack, or both, add support for the node: protocol. Moving forward this will become a bigger issue—there are packages that aren’t even available without the node: protocol.

OskarGroth commented 2 years ago

Thanks, it seems NextJS have now resolved this issue, I'm no longer seeing this error. Closing.

leafac commented 2 years ago

Awesome. Thanks for looking into this.