vercel / next.js

The React Framework
https://nextjs.org
MIT License
122.59k stars 26.23k forks source link

Add sharp to next's devDependencies #67152

Closed joulev closed 1 day ago

joulev commented 4 days ago

Why?

Currently, for a freshly installed clone of the Next.js repo in a fresh machine (no pnpm cache), running pnpm build will fail because sharp is not automatically installed (GitHub Codespaces, Node.js v20.2.0, pnpm v8.15.7).

pnpm build log ``` ┌ next#build > cache bypass, force executing 90354ed2b7dda4a9 │ │ > next@15.0.0-canary.42 build /workspaces/next.js/packages/next │ > pnpm release │ │ │ > next@15.0.0-canary.42 release /workspaces/next.js/packages/next │ > taskr release │ │ │ │ > next@15.0.0-canary.42 types /workspaces/next.js/packages/next │ > tsc --declaration --emitDeclarationOnly --stripInternal --declarationDir dist │ │ src/server/image-optimizer.ts:43:27 - error TS2307: Cannot find module 'sharp' or its corresponding type declaration │ s. │ │ 43 let _sharp: typeof import('sharp') │ ~~~~~~~ │ │ │ Found 1 error in src/server/image-optimizer.ts:43 │ │  ELIFECYCLE  Command failed with exit code 2. │ [13:14:54] generate_types failed because Command failed with exit code 2 (ENOENT): pnpm run types │ [13:14:54] Task chain was aborted! │ [13:14:54] Finished build in 42.53s │ [13:14:54] Finished release in 43.23s │  ELIFECYCLE  Command failed with exit code 1. │  ELIFECYCLE  Command failed with exit code 1. │ command finished with error: command (/workspaces/next.js/packages/next) /home/node/.local/share/pnpm/pnpm run build │ exited (1) └────> ```

It could be a skill issue on my part, but if not, this certainly is a source of confusion for anyone trying to install the repository.

So I add sharp to devDependencies so that it will be installed all of the time for anyone running pnpm i, and the typing of sharp is guaranteed to be available.

ijjk commented 4 days ago

Allow CI Workflow Run

Note: this should only be enabled once the PR is ready to go and can only be enabled by a maintainer

joulev commented 2 days ago

I'm using pnpm 8.15.7.

(Other pnpm versions actually refuses to run because of the explicit "packageManager": "pnpm@8.15.7" in package.json, as expected.)

However the system I'm using (GitHub Codespaces) looks like it doesn't support a prebuilt binary of sharp, that's why the optional dependency is not installed automatically. I also verified that in a blank Next.js app:

@joulev ➜ /workspaces/test-sharp (main) $ pnpm build

> test-sharp@0.1.0 build /workspaces/test-sharp
> next build

  ▲ Next.js 15.0.0-canary.46

   Creating an optimized production build ...
Failed to compile.

./app/pfp.jpeg
Error: Module `sharp` not found. Please run `npm install --cpu=wasm32 sharp` to install it.

But because the optional dependency is not installed, pnpm build of the Next.js source will fail since the typing of the sharp package is needed during build, which blocks development.

So I think it would be a good idea to force-install the typing of sharp as devDependencies to, at least, unblock development for those using similar architectures/systems (Codespaces in this case) and not working on areas related to next/image handling.

eps1lon commented 2 days ago

So I think it would be a good idea to force-install the typing of sharp as devDependencies to

If it fails installing in optional dependencies, wouldn't it also fail in devDependencies? I'd prefer the hard error here since you really can't do much without pnpm build. But moving the dependency won't fix pnpm build.

joulev commented 1 day ago

Indeed this approach is not the fit-all solution. Force-installing sharp as devDependencies does give the typing of sharp, which makes pnpm build pass type-checking; but at the same time, the behaviour during runtime may not be correct.

However, I see that, for supported architectures, this change shouldn't have any effects, since the correct binary of sharp is still installed anyway. Same for end-developers, because changes in devDependencies shouldn't influence anything there.

Meanwhile, for unsupported architectures, if the correctness of sharp at runtime is not required (e.g. when developing an unrelated part of the repo), this change should unblock next build failing, at the cost that if someone develops something related to next/image in an unsupported system, they will get glitchy behaviours.

The chance of someone developing next/image in an unsupported system (which would fail anyway with or without this change) is quite low I think. Meanwhile the chance of someone developing anything in an unsupported system is not so low, since GitHub Codespaces is surprisingly one of those unsupported systems and plenty of people use it. So overall I think the benefit outweighs the cost.

The perfect solution to this, imo, would be to still install typings for sharp like this, but in runtime if the correct binary of sharp is not found, then an error message is thrown. Though I'm not entirely sure if that's doable.

What do you think?

joulev commented 1 day ago

Now that I've thought a bit more about it, this looks like a good direction too: sharp is still only an optionalDependencies, but in the building/developing instructions for contributors, add a note on manually installing the wasm version of sharp there for systems that fail to automatically install the library.

styfle commented 1 day ago

The error message is already there when you pnpm install next as seen in your previous comment: https://github.com/vercel/next.js/pull/67152#issuecomment-2193995664

Error: Module sharp not found. Please run npm install --cpu=wasm32 sharp to install it.

I see what you mean that contributors to the Next.js source code, the error isn't as obvious. Feel free to create another PR to update these docs, thanks!

Closing this one since its not the right solution.