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

Incorrect use of "module"/ESM in package.json #1110

Closed
danielholmes opened this issue Nov 21, 2022 · 3 comments · Fixed by #1149
Closed

Incorrect use of "module"/ESM in package.json #1110

danielholmes opened this issue Nov 21, 2022 · 3 comments · Fixed by #1149
Labels
help wanted released This issue/pull request has been released.

Comments

@danielholmes
Copy link
Collaborator

Expected Behavior

I expect the ESM bundle to include valid ESM code and thus when building with an ESM bundler or interacting with ESM based tools, it should work.

Current Behavior

ESM bundlers (such as rollup.js) produce a failure.

Failure Information (for bugs)

ES modules were added to the Javascript spec in 2015. To make use of them, people started adopting the "module" field in package.json. i.e. the following package.json excerpt tells users of this package to import es/index.js if they require an ESM compatible source, otherwise use dist/index.js:

{
  "name": "jimp",
  "version": "0.16.2",
  "main": "dist/index.js",
  "module": "es/index.js",
  ...
 }

The problem in JIMP is that es/index.js isn't an ES module at all (View source) - it's a JS file using the CommonJS module format. This format was created for node.js but there have been a lot of tools such as browserify and babel created which allow you to use this format in the browser.

There are various good technical reasons to want to support ESM. But from a practical standpoint there are a wave of tools gaining popularity which are built around ESM and JIMP will be left behind without properly supporting it (e.g. vite, esbuild, rollup).

Steps to Reproduce

I've created a minimal repo here: https://github.com/danielholmes/jimp-rollup which shows the problem in the rollup context.

  1. Clone repo
  2. Install dependencies (npm ci)
  3. Run a build npm run build

Context

  • Jimp Version: 0.16.2
  • Operating System: Mac OS and Ubuntu (should be all platforms, but didn't check any others)
  • Node version: 18.12.1 (should be all though)

Failure Logs

> n run build

> jimp-rollup@1.0.0 build
> rollup -c


main.js → bundle.js...
[!] RollupError: "default" is not exported by "node_modules/jimp/es/index.js", imported by "main.js".
https://rollupjs.org/guide/en/#error-name-is-not-exported-by-module
main.js (1:7)
1: import Jimp from "jimp";
          ^
2: 
3: const blankImage = Jimp.create(100, 100, 0x00000000);
RollupError: "default" is not exported by "node_modules/jimp/es/index.js", imported by "main.js".
    at error (~/jimp-rollup/node_modules/rollup/dist/shared/rollup.js:206:30)
    at Module.error (~/jimp-rollup/node_modules/rollup/dist/shared/rollup.js:13355:16)
    at Module.traceVariable (~/jimp-rollup/node_modules/rollup/dist/shared/rollup.js:13718:29)
    at ModuleScope.findVariable (~/jimp-rollup/node_modules/rollup/dist/shared/rollup.js:12270:39)
    at MemberExpression.bind (~/jimp-rollup/node_modules/rollup/dist/shared/rollup.js:9439:49)
    at CallExpression.bind (~/jimp-rollup/node_modules/rollup/dist/shared/rollup.js:6089:23)
    at CallExpression.bind (~/jimp-rollup/node_modules/rollup/dist/shared/rollup.js:9765:15)
    at VariableDeclarator.bind (~/jimp-rollup/node_modules/rollup/dist/shared/rollup.js:6089:23)
    at VariableDeclaration.bind (~/jimp-rollup/node_modules/rollup/dist/shared/rollup.js:6085:28)
    at Program.bind (~/jimp-rollup/node_modules/rollup/dist/shared/rollup.js:6085:28)
@danielholmes
Copy link
Collaborator Author

danielholmes commented Nov 24, 2022

I made a small repo to try and get jimp working with vite (requires proper ESM setup) and I was able to get it working. See:

https://github.com/danielholmes/jimp-vite for the code and https://danielholmes.github.io/jimp-vite/ for the bundled example.

Some notes on how I got it working:

  1. I used a custom packaging of jimp to strip all features except jpeg type support. This is just to prove the concept - the idea would be to add back plugins one-by-one and fix the related issues as they come up.
  2. I added polyfills for the NodeJS modules in the vite config. This is a usual thing to have to do with vite so doesn't represent anything to change in the jimp code. I also had to add a runtime polyfill for NodeJS process, again I'd say not something to necessarily change in the jimp code.
  3. I used patch-package for the jimp packages to use the original repository src directory instead of the distributed es directory. This is for the 3 packages I used in this example - @jimp/custom, @jimp/core and @jimp/jpeg .
  4. I used patch-package to change the src for @jimp/core/src/request.js, which is in commonJS format not ESM.

I would be interested in submitting a PR with the relevant changes to get these packages working. So far it would be mostly simple changes:

  1. Change the jimp bundling script to copy src to es rather than perform a babel process.
  2. Update JIMP src to be fully ESM (only found @jimp/core/src/request.js that wasn't atm)

I'm a little concerned that it might break developer's build systems that use these packages - I do think it should be classed as a breaking change.

@hipstersmoothie
Copy link
Collaborator

@danielholmes if you're still interested in fixing this I'm up for the PRs. As for breaking changes I'm fine with it for now. I plan to make a bunch of smaller breaking changes 0.x before a v1 release

@github-actions
Copy link
Contributor

github-actions bot commented Feb 5, 2023

🚀 Issue was released in v0.21.0 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted released This issue/pull request has been released.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants