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

No viable alternative to imagemin - applying for maintainer privileges #409

Closed
webketje opened this issue Jan 17, 2023 · 9 comments
Closed

Comments

@webketje
Copy link
Member

Hello there,

Since Squoosh completely removed their @lib/squoosh / CLI package 2 weeks ago, the recommendation @sindresorhus gave in #385 no longer holds and Node.js users seeking a library for compressing images as part of a build pipeline are effectively left without any options.

As the maintainer of metalsmith and supporting the metalsmith-imagemin plugin,
I would like to apply for maintainer privileges in the imagemin org + on npmjs.org (at least for the pngquant, svgo, webp, jpegoptim plugins).

I am aware pre-built binaries included in NPM packages come with their own set of issues.
However, I want to explore enabling users to take advantage of the easy-to-use interface of the imagemin packages while making using the prebuilt binaries optional.

My scope of involvement would be:

  • Adding missing Typescript definitions
  • Exploring making the *-bin libraries optional peerDependencies of the imagemin-* plugins and making relying on the OS-installed dependencies the default (eg for using webp on Debian distro's, try calling webp first, which will fail if it is not in $PATH).
  • Allowing users to override the default executable path with an option executablePath.
  • Publishing patch releases for security fixes

I'll adhere to the conventions in place (linters, esm-only, etc..)

@sindresorhus
Copy link
Contributor

Invited you 👍

@sindresorhus
Copy link
Contributor

Exploring making the -bin libraries optional peerDependencies of the imagemin- plugins and making relying on the OS-installed dependencies the default (eg for using webp on Debian distro's, try calling webp first, which will fail if it is not in $PATH).

There are problems with relying on OS-installed dependencies too, like version incompatibilities.

I would suggest exploring using WASM for some of these as it fixes almost all compatibility problems.

For example, pngquant can be used from WASM: https://github.com/GoogleChromeLabs/squoosh/tree/dev/codecs/imagequant

@Tofandel
Copy link

Also if you publish a major, can you make the package cjs compatible with an exports in the package.json, it was recently made esm only and it's unpractical because the main usage of this package is in build tools which don't just don't have good esm support

@webketje
Copy link
Member Author

@Tofandel TBH this was also an issue for metalsmith-imagemin (which I pinned to the latest CJS-compatible imagemin plugins for my fork) as long as you support Node 12. FWIW you can use ESM modules in CJS builds by using dynamic imports starting in Node 13.something. Providing a dual build is definitely outside the scope of my involvement, but I could clarify in the README how to load it in a CJS build.

@sindresorhus last thing: do you have a preferred Way of Working? Contributor's guide with maintainer section somewhere? Should I create branches and add you/someone as reviewer? Who pubs to NPM? I know you're super-busy, so I'm not sure you wish to be pinged for imagemin-related stuff. Perhaps you can point me to another active maintainer? 😄

@Tofandel
Copy link

Tofandel commented Jan 21, 2023

You don't need a dual build but you do need a typescript base or a build step, I did it for @prerenderer by simply giving a cjs.js file which imports the entry of the module

"use strict";
var __importDefault = (this && this.__importDefault) || function (mod) {
    return (mod && mod.__esModule) ? mod : { "default": mod };
};
const index_1 = __importDefault(require("./index"));
module.exports = index_1.default;

Or before building in ts

import def from './index'

export = def

And then in the package.json remove the type: "module" and add

  "exports": {
    "import": "./dist/index.js",
    "require": "./dist/cjs.js"
  },

Works wonders, as this allows for both require and import in node builds instead of just the import that the type: "module" does

@sindresorhus
Copy link
Contributor

I don't intend to bring back CommonJS support.

@sindresorhus
Copy link
Contributor

sindresorhus commented Jan 22, 2023

last thing: do you have a preferred Way of Working? Contributor's guide with maintainer section somewhere? Should I create branches and add you/someone as reviewer? Who pubs to NPM? I know you're super-busy, so I'm not sure you wish to be pinged for imagemin-related stuff. Perhaps you can point me to another active maintainer?

I don't have a guide. Just the normal best practices. New features should have tests. It's good to get a review on larger changes. Smaller changes can land directly on main. Feel free to add me as a reviewer. There are no other active maintainers. It's easy to get burnt out on this project. If you have someone else that would be interested in reviewing your changes, that would be great too. You are free to publish new releases on npm.

@webketje
Copy link
Member Author

webketje commented Feb 2, 2023

@ahmadnassri this thread should be of interest to you

Sindre mentioned

There are no other active maintainers. It's easy to get burnt out on this project. If you have someone else that would be interested in reviewing your changes, that would be great too.

Are you interested? If not I'm going to close this issue since I got invited to the GH org.

@ahmadnassri
Copy link

@webketje thanks for the follow up, I don't think I can help with with a larger scope project like imagemin at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants