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

feat(next/image)!: remove squoosh in favor of sharp as optional dependency #63321

Merged
merged 13 commits into from
Apr 25, 2024

Conversation

styfle
Copy link
Member

@styfle styfle commented Mar 15, 2024

History

Previously, we added support for squoosh because it was a wasm implementation that "just worked" on all platforms when running next dev for the first time. However, it was slow so we always recommended manually installing sharp for production use cases running next build and next start.

Now that sharp supports webassembly, we no longer need to maintain squoosh, so it can be removed. We also don't need to make the user install sharp manually because it can be installed under optionalDependencies. I left it optional in case there was some platform that still needed to manually install the wasm variant with npm install --cpu=wasm32 sharp such as codesandbox/stackblitz (I don't believe sharp has any fallback built in yet).

Since we can guarantee sharp, we can also remove get-orientation dep and upgrade image-size dep.

I also moved an existing sharp test into its own fixture since it was unrelated to image optimization.

Related Issues

Breaking Change

This is a breaking change because newer versions of sharp no longer support yarn@1.

The workaround is to install with yarn --ignore-engines flag.

Also note that Vercel no longer defaults to yarn when no lockfile is found

Closes NEXT-2823

@styfle styfle requested review from ismaelrumzan and StephDietz and removed request for a team March 15, 2024 13:06
Copy link

vercel bot commented Mar 15, 2024

Notifying the following users due to files changed in this PR based on this repo's notify modifiers:

@timneutkens, @ijjk, @shuding:

packages/next/src/server/lib/squoosh/.vercel.approvers
packages/next/src/server/lib/squoosh/LICENSE
packages/next/src/server/lib/squoosh/avif/avif_enc.d.ts
packages/next/src/server/lib/squoosh/avif/avif_node_dec.js
packages/next/src/server/lib/squoosh/avif/avif_node_dec.wasm
packages/next/src/server/lib/squoosh/avif/avif_node_enc.js
packages/next/src/server/lib/squoosh/avif/avif_node_enc.wasm
packages/next/src/server/lib/squoosh/codecs.ts
packages/next/src/server/lib/squoosh/emscripten-types.d.ts
packages/next/src/server/lib/squoosh/emscripten-utils.ts
packages/next/src/server/lib/squoosh/image_data.ts
packages/next/src/server/lib/squoosh/impl.ts
+18 more

Copy link

socket-security bot commented Mar 15, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/[email protected] filesystem +2 71.6 kB netroy
npm/[email protected] environment Transitive: filesystem, shell +10 764 kB lovell

🚮 Removed packages: npm/@types/[email protected], npm/[email protected], npm/[email protected], npm/[email protected]

View full report↗︎

@ijjk ijjk added the tests label Apr 25, 2024
Copy link

socket-security bot commented Apr 25, 2024

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

Ignoring: npm/[email protected]

View full report↗︎

Next steps

Take a deeper look at the dependency

Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev.

Remove the package

If you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency.

Mark a package as acceptable risk

To ignore an alert, reply with a comment starting with @SocketSecurity ignore followed by a space separated list of ecosystem/package-name@version specifiers. e.g. @SocketSecurity ignore npm/[email protected] or ignore all packages with @SocketSecurity ignore-all

@styfle
Copy link
Member Author

styfle commented Apr 25, 2024

@SocketSecurity ignore npm/[email protected]

@styfle styfle merged commit a6a6117 into canary Apr 25, 2024
82 of 83 checks passed
@styfle styfle deleted the remove-squoosh-add-sharp-optionaldep2 branch April 25, 2024 18:01
@karlhorky
Copy link
Contributor

@styfle Thanks for this! 🙌

This was released in [email protected], I can confirm that this is now installing sharp in a deep directory node_modules/.pnpm/[email protected][email protected][email protected][email protected]/node_modules/sharp/package.json:

➜  p mkdir a
➜  p cd a
➜  a pnpm create [email protected] . --app --no-eslint --no-src-dir --import-alias @/\* --no-tailwind --typescript
.../18f19519fb1-1452a                    |   +1 +
.../18f19519fb1-1452a                    | Progress: resolved 1, reused 0, downloaded 1, added 1, done
Creating a new Next.js app in /Users/k/p/a.

Using pnpm.

Initializing project with template: app


Installing dependencies:
- react
- react-dom
- next

Installing devDependencies:
- typescript
- @types/node
- @types/react
- @types/react-dom

Packages: +40
++++++++++++++++++++++++++++++++++++++++
Downloading @next/[email protected]: 37.39 MB/37.39 MB, done
Downloading [email protected]: 23.71 MB/23.71 MB, done
Progress: resolved 66, reused 37, downloaded 4, added 40, done

dependencies:
+ next 14.3.0-canary.24
+ react 18.3.0
+ react-dom 18.3.0

devDependencies:
+ @types/node 20.12.7
+ @types/react 18.3.0
+ @types/react-dom 18.3.0
+ typescript 5.4.5

Done in 12.8s
Initialized a git repository.

Success! Created a at /Users/k/p/a

➜  a git:(main) cat node_modules/.pnpm/[email protected][email protected][email protected][email protected]/node_modules/sharp/package.json
{
  "name": "sharp",
  "description": "High performance Node.js image processing, the fastest module to resize JPEG, PNG, WebP, GIF, AVIF and TIFF images",
  "version": "0.33.3",
  ...

One thing that was a bit surprising was that sharp was not present in node_modules/sharp, but I guess that's a detail of how pnpm does hoisting of optional dependencies:

➜  a git:(main) ✗ ls node_modules/sharp
ls: node_modules/sharp: No such file or directory

@karlhorky
Copy link
Contributor

karlhorky commented Apr 26, 2024

Looking forward to the stable release (probably will be in the next major):

@styfle
Copy link
Member Author

styfle commented Apr 26, 2024

One thing that was a bit surprising was that sharp was not present in node_modules/sharp, but I guess that's a detail of how pnpm does hoisting of optional dependencies:

That's how pnpm works. You can shamefully-hoist if you want pnpm to behave like npm and hoist dependencies up.

Looking forward to the stable release (currently assuming this will be in the next minor):

It will will likely be [email protected] since dropping yarn@1 could be considered a breaking change. I suppose it could be minor if we think that yarn@1 usage is nearing zero but we would have to look at telemetry (not sure if we currently collect that data). Or we could consider it non-breaking since yarn@1 has a workaround but that might be painful to add the --ignore-engines flag.

@karlhorky
Copy link
Contributor

karlhorky commented Apr 26, 2024

Great, updated my post above, thanks!

will likely be [email protected] since dropping yarn@1 could be considered a breaking change

Yeah totally understand here - Yarn v1 is still entrenched in a lot of places. (even for some of our apps still on our list to upgrade)

@Netail
Copy link
Contributor

Netail commented May 7, 2024

Since 14.3.0-canary.24 (The release after this PR got merged) it seems like NextJS throws the following on macOS;

Error: Could not load the "sharp" module using the darwin-arm64 runtime
web:dev: Possible solutions:
web:dev: - Ensure optional dependencies can be installed:
web:dev:     npm install --include=optional sharp
web:dev:     yarn add sharp --ignore-engines
web:dev: - Ensure your package manager supports multi-platform installation:
web:dev:     See https://sharp.pixelplumbing.com/install#cross-platform
web:dev: - Add platform-specific dependencies:
web:dev:     npm install --os=darwin --cpu=arm64 sharp
web:dev: - Consult the installation documentation:
web:dev:     See https://sharp.pixelplumbing.com/install
web:dev: Import trace for requested module:
web:dev: ../../node_modules/next/dist/build/webpack/loaders/next-metadata-image-loader.js?type=favicon&segment=&basePath=&pageExtensions=tsx&pageExtensions=ts&pageExtensions=jsx&pageExtensions=js!./app/favicon.ico?__next_metadata__

@styfle
Copy link
Member Author

styfle commented May 7, 2024

@Netail See the "Breaking Change" section of the PR description above

@Netail
Copy link
Contributor

Netail commented May 7, 2024

@Netail See the "Breaking Change" section of the PR description above

Ahhh oke, interesting. That's gonna break a lot of frontends (And kind of annoying in terms of DX), as Yarn v1 is still used by 2.5 million people...

Screenshot 2024-05-07 at 22 54 55

@styfle
Copy link
Member Author

styfle commented May 7, 2024

Thats not 2.5 million people, thats 2.5 million installs of [email protected].

For comparison, the latest version of [email protected] that came out 7 days ago is already over a million installs.

image

As mentioned above, if you wish to continue using yarn@1, you can use the yarn --ignore-engines flag.

@Netail
Copy link
Contributor

Netail commented May 7, 2024

Ah yes your're correct, I meant installs. But I feel like the flag --ignore-engines is a hacky workaround, which could cause issues with other packages and will confuse a lot of developers who didn't read up on this?

@Netail
Copy link
Contributor

Netail commented May 7, 2024

Also this error is shown even tho I do not make use of the next image component, this happens with a clean create-next-app + a canary version of 14.3.0-canary.24 or higher

@styfle
Copy link
Member Author

styfle commented May 7, 2024

the flag --ignore-engines is a hacky workaround, which could cause issues with other packages

Which issues are you seeing and with which packages?

I do not make use of the next image component, this staight after a clean create-next-app

The default create-next-app does indeed use next/image

@Netail
Copy link
Contributor

Netail commented May 7, 2024

the flag --ignore-engines is a hacky workaround, which could cause issues with other packages

Which issues are you seeing and with which packages?

None right now, but could occur I guess

I do not make use of the next image component, this staight after a clean create-next-app

The default create-next-app does indeed use next/image

Sorry, not entirely clear. Got rid of the page contents, but no usage of next/image

@styfle
Copy link
Member Author

styfle commented May 7, 2024

Sorry, not entirely clear. Got rid of the page contents, but no usage of next/image

In that case, you can ignore the warning because sharp is an optional dependency so you can continue using next even if sharp is not installed.

@Netail
Copy link
Contributor

Netail commented May 7, 2024

Sorry, not entirely clear. Got rid of the page contents, but no usage of next/image

In that case, you can ignore the warning because sharp is an optional dependency so you can continue using next even if sharp is not installed.

Well I can't ignore it, as it can't even compile the app...
Screenshot 2024-05-07 at 23 30 15

(I think there was a misunderstanding about the yarn part, as I thought it was for installing next, while you were talking about installing sharp. But I am facing an issue when not even using next/image)

@styfle
Copy link
Member Author

styfle commented May 7, 2024

I see the problem now. We need to lazy load it so you only get the error when using image optimization.

This should fix it: #65484

@Netail
Copy link
Contributor

Netail commented May 7, 2024

I see the problem now. We need to lazy load it so you only get the error when using image optimization.

This should fix it: #65484

Lovely, thanks :)

styfle added a commit that referenced this pull request May 21, 2024
Users no longer need to install `sharp` manually thanks to the
following:

- #63321
styfle added a commit that referenced this pull request May 22, 2024
This parameter was previously used for a warning message, but we no
longer print that warning since
#63321 so now we can remove the
unused parameter.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
created-by: Next.js team PRs by the Next.js team. Documentation Related to Next.js' official documentation. locked tests type: next
Projects
None yet
6 participants