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

fix: Remove usage of p-map #281

Closed
wants to merge 1 commit into from

Conversation

AbhiPrasad
Copy link

@AbhiPrasad AbhiPrasad commented Jul 2, 2024

When investigating reducing the dependency count for the semantic-release library, I discovered that npm has quite the large dependency tree: es-tooling/ecosystem-cleanup#64

One area of improvement here is to remove the dependency on the p-map library and replace it with a smaller, simpler helper.

Right now cacache relies on [email protected], which pulls in [email protected]. Newer versions of p-map don't pull this in aggregate-error at all, but that is not feasible to use because this cacache is CJS only, and p-map 5.x and above are ESM only.

Given cacache supports "^16.14.0 || >=18.0.0" as per it's package.json engines field, it doesn't make sense for it to pull in a polyfill for aggregate-error (Node.js 15+ adds support for the built-in AggregateError.

To replace pMap which we used from the p-map library, I introduced a mapWithPromise helper. It doesn't have all the functionality of pMap, but it works well for the use cases in lib/verify.js. In addition it should be slightly faster than pMap because of it is simpler. (let me know and I'm happy to bench mark to get exact numbers).

This change should help eliminate 25 KB / 4 deps from the module graph of cacache.

References

Supercedes #266

@wraithgar
Copy link
Member

p-map being esm-only does not preclude updating to it here. The blocker is engines. Once we bump that in this repo we can update. Generally we only replace a library and roll our own solution when we absolutely have to. I don't think we will be accepting this PR, sorry.

@wraithgar wraithgar closed this Jul 2, 2024
@AbhiPrasad
Copy link
Author

The blocker is engines

Could you clarify why the blocker is engines? Are you waiting for require(esm) support?

@wraithgar
Copy link
Member

p-map does not work in node 16, cacache does.

~ $ npm view cacache engines
{ node: '^16.14.0 || >=18.0.0' }
~ $ npm view p-map engines
{ node: '>=18' }

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.

None yet

2 participants