unifiedjs / unified

☔️ interface for parsing, inspecting, transforming, and serializing content through syntax trees
https://unifiedjs.com
MIT License
4.49k stars 110 forks source link

remove `is-plain-obj` deps #248

Closed Jayllyz closed 3 months ago

Jayllyz commented 3 months ago

Initial checklist

Problem

https://e18e.dev/guide/cleanup.html https://packages.ecosyste.ms/registries/npmjs.org/packages/is-plain-obj/dependent_packages?order=desc&sort=downloads Unified is top 2 dependant of is-plain-obj

Solution

Replace by a oneline function :

function isPlainObject(v) {
    return v && typeof v === 'object' && (Object.getPrototypeOf(v) === null || Object.getPrototypeOf(v) === Object.prototype);
}

Alternatives

.

wooorm commented 3 months ago

Hey Anthony!

And then?

Do you use unified?

How far do you want to go? Do you want to inline every dependency?


Please at least put some time in your issue and respect our time too. There are of course alternatives and trade offs.

wooorm commented 3 months ago

How well does your code work? I’d assume it might break across realms, due to the Object.prototype?

Jayllyz commented 3 months ago

Hello, thanks for the fast answer.

To be honest, no I don't use unified. I'm just trying to help clean up the js ecosystem even if my contribution is a grain of salt.

"Inline every dependency" isn't what we're trying to do, but some packages shouldn't exist. Take a look at this PR where a contributor saves tons of data by removing is-number as ex. https://github.com/micromatch/to-regex-range/pull/17

This oneline comes from :

https://github.com/es-tooling/module-replacements/blob/main/manifests/micro-utilities.json

If you don't want to change it that's fine, don't worries.

remcohaszing commented 3 months ago

I personally don’t really think this check is very useful: https://github.com/unifiedjs/unified/blob/35e72b97b067f697730db18b8a902a40065882b0/lib/index.js#L1254-L1261

But I don’t think there’s a way around this check: https://github.com/unifiedjs/unified/blob/35e72b97b067f697730db18b8a902a40065882b0/lib/index.js#L1165

I agree removing useless dependencies is a good thing. I don’t necessarily think this is a completely useless dependency. You should look at why a dependency is used and come up with a proposal to replace it. A suggestion to “remove dependency X isn’t helpful.

wooorm commented 3 months ago

It’s used in case 1 because the dep is already there. The dep is there because of case 2.

ChristianMurphy commented 3 months ago

If there is a runtime performance reason, runtime correctness reason, or security reason to drop the dependency I'm all for it.

The premise of minifying code or documentation to save bandwidth downloading to a developer machine is not one I subscribe to. Developers should have access to clear easy-to-understand code and documentation. Code and documentation downloaded from a package manager is cached, it's a one time cost.

For folks who are concerned with the bandwidth, take it up with CI/CD providers and NPM itself to offer more tailored download options, rather than going after individual projects over a few kilobytes.

Jayllyz commented 3 months ago

Thanks for your feedbacks, I see that we have different opinions about such packages (even within e18e)

I don't want to get into a drama/debate because that's not the aim of this initiative. If you want to help or suggest things, I recommend you go to the discord.

Thank you, all the best for your projects and sorry for the time lost.

github-actions[bot] commented 3 months ago

Hi! This was closed. Team: If this was fixed, please add phase/solved. Otherwise, please add one of the no/* labels.

github-actions[bot] commented 3 months ago

Hi team! I don’t know what’s up as there’s no phase label. Please add one so I know where it’s at.

Thanks, — bb