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

Unofficial updates #120

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Unofficial updates #120

wants to merge 8 commits into from

Conversation

webketje
Copy link

No description provided.

"imagemin-svgo": "^7.0.0",
"imagemin-webp": "^5.0.0",
"imagemin-zopfli": "^6.0.0"
"imagemin": "^7.0.1",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the latest versions of these packages have all been switched to ESM modules, which won't work with this CJS module, hence why I kept them at the specific versions prior to ESM conversion...

what are your thoughts on ESM vs. CJS for Metalsmith?

Copy link
Author

@webketje webketje Dec 20, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not all. I tested and reviewed their GH pages one-by-one and upgraded to the latest CJS-compatible versions. I may have missed one. The number of security warnings metalsmith-imagemin gave was too high to ignore (76), let alone include in a build in a professional settting. It still has about 30 I think, but there is little you or me can do but use package.json overrides in each project.

Regarding ESM/CJS all core plugins are (on the way to be) distributed as dual ESM/CJS packages with TS support. I wrote a news about it recently. Using dynamic import may be a better future alternative than using a bundler (because it is supported in both CJS/ESM environments). But CJS will be supported as long as metalsmith supports versions of Node where { "type": "module" } is not the default (= a very long time).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: getting npm install failures here...

image

seems like imagemin-advpng does a postinstall step where it downloads a binary using got, one needs to delete node_modules folder and package-lock.json completely run npm i again after every change of those package versions just to make sure everything works...

doing so, shows the breakage with [email protected] override.

unfortunately the latest non-esm version of got is v11.8.6 which cannot be used in this override either, as it causes jpeg-recompress to fail pre-build step ...

image

...

I think the only next step (to avoid all the vuln warnings) I could convert this whole package to a dual esm / cjs compiled package and make it work on both ...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I poked sindre on twitter, and he linked me to this: imagemin/imagemin#385

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like imagemin is dead and we shouldn't expect anything to work in these dependencies ... so the ESM conversion is needed (but likely wont stop future vulns)

I'll continue the esm conversion, but then also look into replacing the internals with https://github.com/GoogleChromeLabs/squoosh

Copy link
Author

@webketje webketje Dec 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed and was writing a reply but forgot to hit comment:
The problem is the imagemin project itself is actually unmaintained, and the lib it was based on is unmaintained too. But the underlying binaries are still actively developed (jpegoptim, advpng etc).
So indeed Squoosh would be a better alternative, or forking the dependency chain of imagemin (for each plugin: imagemin-<plugin>, <plugin>-bin, bin-wrapper). A.o. this would allow configuring using a dynamically linked binary (instead of the ones shipped with imagemin plugins) which is suitable for custom devservers (but not for the likes of Netlify or Github Pages)

Still there's ~5 commits in this PR you could cherry-pick and apply for a final CJS-compatible release.

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

Successfully merging this pull request may close these issues.

2 participants