Conversation
| } | ||
|
|
||
| if (sharp) { | ||
| module.exports = sharp; |
There was a problem hiding this comment.
Conditional exports are not supported by ESM but it doesn't matter here because importing the module will throw as needed
|
This is great, thank you for doing the hard work Florian, and at less of an impact in terms of changed code than I had imagined, merci donc d'avoir fait simple. For the v0.35.x releases I'd like to maintain compatibility with versions of Node.js where ESM isn't fully available and then I suspect the v0.36.x releases (or maybe v1.0.x if I'm feeling lucky after 13 years) is the place for this to land and wish CommonJS bon voyage. As an aside, I've been invited to (and accepted) the Astro event in London in a few weeks and spotted your name on the list so I might be able to thank you in person for this. |
|
Glad I can help :) Yeah I saw you on the list for the event, looking forward to meeting you! I'll give a talk on fonts Btw I was thinking about this PR during the weekend and I don't know if the packages under |
|
@lovell can you approve the workflow to see if the tests pass? |
|
To keep this PR as minimal as possible, I switched from ESM first + CJS compat to CJS first + ESM compat, which required a few more changes. Workflow should run better next time you approve it! |
|
Build and tests are now passing locally! |
| ], | ||
| "scripts": { | ||
| "build": "node install/build.js", | ||
| "build:dist": "node scripts/build.mjs", |
There was a problem hiding this comment.
I had to add another script for lib to dist compilation because if I understood correctly, sharp can be built from source by doing npm explore sharp -- npm run build. If the compilation happened in this script, it would fail since mjs files in lib are not shipped to NPM anymore
|
I swear this should work now |
|
Can we remove CJS completely from the published npm package now that you can https://joyeecheung.github.io/blog/2025/12/30/require-esm-in-node-js-from-experiment-to-stability/ Node.js 18 has been EOL for a year now: https://nodejs.org/en/about/previous-releases#looking-for-the-latest-release-of-a-version-branch |
|
My first thought was also to move to ESM-only but we can't unfortunately (yet). The problem is CJS cannot require ESM with TLAs, which is currently used to load the native binaries. This PR aims to be as small/compatible as possible. Once this is released, I'll make another PR for another release (maybe 1.0?) to move to ESM-only. To be clear, that means that in the current state it could not be used by CJS at all. Maybe that's fine, otherwise it will require a change in the public API, e.g.: -import sharp from 'sharp'
+import loadSharp from 'sharp'
+const sharp = await loadSharp() // native binaries loading
await sharp(buffer).resize()I think 1.0 would be a good fit for ESM-only + API change + Node engine requirement change |
Whats the purpose of the intermediary step though? Its making sharp (thats downloaded 53M times per week) larger and sounds like it doesn't solve the original issue (making sharp work in non-CJS environments). |
|
Supporting both at the same time seemed to be Lovell's preferred solution on the issue so that's why. I don't have strong opinions on whether that's not worth doing. At the very least it's useful as research
This PR makes it work perfectly fine in ESM and CJS, but yeah with tradeoff of making it larger |
|
@styfle @florian-lefebvre Thanks both, a move to ESM-only as part of a major version bump feels like the way to go here. I still think it's worth providing support for both CJS and ESM in v0.35.0, as Florian has kindly implemented in this PR. For those who care about installation size (I know you do Steven 😅 ) then the biggest issue with the next release is going to be via an npm bug that erroneously installs transitive dependencies of unmet optional dependencies - see npm/cli#8832 (comment) |
|
Alright I fixed everything so it should work fine against the 0.35 branch now! |
Closes #2981
I kept this PR as minimal as I could. It introduces esbuild to be able to compile ESM to CJS to support both:
libto ESM because esbuild doesn't support transformingrequire()intoimportrequire()Things left to do where I'll need some help:
install/build.jsrequire()calls inlibvips.jsbecause they only seem consumed bybinding.gyp