What the motivation of this fork?

[JounQin]

Is there any related discussion contexts?

cc @fisker @sindresorhus

[lishaduck]

Nobody's watching this repo, so I assume @43081j is unaware this was raised.
I'm also curious, so hopefully that pinged him.

[JounQin]

@lishaduck See https://discord.com/channels/1227627367204257852/1365520775637106788

[lishaduck]

Can you paste in a screenshot or whatever for those of use who won't use dis?

[JounQin]

Can you paste in a screenshot or whatever for those of use who won't use dis?

Sorry but no, the context is too long.

[43081j]

Sorry folks, I'll write up some reasoning today

Importantly, this hasn't been published yet and some of it will probably be used to open upstream PRs later

Whether it will exist as it's own fork yet is to be decided (but possible).

[43081j]

ok here you go:

easy wins:

• empathic is faster and smaller than read-package-up (i hear you're moving to find-up-simple, but this will still be true)
• is-builtin-module is unnecessary, node ships a built in list now (we have to exclude punycode since the npm package didn't include it)
• indent-string is unnecessary, especially given it is used to apply a count of indentation but we always call it with 1 and set the indent character to the entire indent string (i.e. the already repeat-ed indent character)
• strip-indent can be replaced with dedent (will be about as big when it switches to ESM-only)
• tinyexec de-dupes better than nano-spawn

more involved wins:

• a custom build of lodash is shipped pretty much just to get some string casing utils - replace this with scule, smaller and faster
• eslint doc generator no longer needs the hack (workaround-for-eslint-doc-generator)

most of this can be contributed back upstream, but i suspect are unlikely to ever be accepted.

i then got carried away and diverged the fork to do a whole bunch of other stuff the original can't feasibly ever really pull in:

• overhaul the dev setup (just rip everything out and use vitest, prettier, simplify lint config, etc)
• remove the custom create wrapper entirely (all it does is reduce CPU perf for sake of giving us some syntactical sugar)

on top of that, i did some quick profiling of the rules and found many places we can probably speed them up or reduce memory consumption. if this turns out to be true, i can try contribute those upstream, but i suspect some optimisations will be too disruptive for the original repo to pull back in.

given that a lot of this may never be accepted into the original repo, i'm happy to keep this fork permanently tracking the original (i.e. always in sync), and at least people have an option to use the lighter/faster one.

[sindresorhus]

It would be great if we could collaborate instead of duplicating efforts over minor differences. 🙏

Forks usually make sense when a project is abandoned or heading in a very different direction. Here, most of the changes seem like small preferences rather than fundamental shifts.

---

empathic is faster and smaller than read-package-up (i hear you're moving to find-up-simple, but this will still be true)

It is done as of v59.

The difference between empathic and find-up-simple in size and speed is neglible in practice. Please read

is-builtin-module is unnecessary, node ships a built in list now (we have to exclude punycode since the npm package didn't include it)

It's useful because it provides a static list, so the result is consistent across Node.js versions: https://github.com/sindresorhus/is-builtin-module?tab=readme-ov-file#is-builtin-module

indent-string is unnecessary, especially given it is used to apply a count of indentation but we always call it with 1 and set the indent character to the entire indent string (i.e. the already repeat-ed indent character)

Happy to have it removed.

strip-indent can be replaced with dedent (will be about as big when it switches to ESM-only)

Unclear what the practical benefit would be?

tinyexec de-dupes better than nano-spawn

It's just a development dependency, so that makes no practical difference. 40 kb is also negible in a non-browser context, and nano-spawn handles more edge-cases than tinyexec.

a custom build of lodash is shipped pretty much just to get some string casing utils - replace this with scule, smaller and faster

Happy to do that as long as they cover all the edge-cases of the Lodash utilities.

but i suspect some optimisations will be too disruptive for the original repo to pull back in

It would be great if you could expand on this. We are very much interested in improving performance.

Keep in mind though that optimizations are only useful if they result in noticable performance difference in real-world projects, not micro-benchmarks.

[43081j]

It is done as of v59.

see this quote from me:

i hear you're moving to find-up-simple, but this will still be true

in case that wasn't written clearly enough, empathic is still faster and smaller so will remain the choice here.

It's useful because it provides a static list, so the result is consistent across Node.js versions

i'm happy to align with the engine being used rather than an arbitrary list.

Unclear what the practical benefit would be?

this is one of the smaller changes. both do roughly the same thing. dedent just happened to de-dupe better in some repos i tried out.

It's just a development dependency, so that makes no practical difference. 40 kb is also negible in a non-browser context, and nano-spawn handles more edge-cases than tinyexec.

i'd like to improve DX for maintainers/contributors, too. so this was a drop-in no-brainer replacement for me. no edge cases nano-spawn apparently handles needed handling here it seems.

Happy to do that as long as they cover all the edge-cases of the Lodash utilities

if somehow they didn't, the correct thing to do is contribute upstream to scule (or whatever utility has been chosen), not to hold on to lodash. but it seems everything works fine with scule anyway

It would be great if you could expand on this. We are very much interested in improving performance

there's more investigation to be done, but i'm happy to try contribute any perf improvements i make here upstream.

Keep in mind though that optimizations are only useful if they result in noticable performance difference in real-world projects, not micro-benchmarks

of course, i performance test a lot of projects, this wasn't in doubt.

given that there won't be agreement on some of these things, i'm happy to maintain the fork and continue to contribute what i can upstream. although the more it diverges, that may become more difficult (though i would still try).

[JounQin]

For devDendpenciea, I don't quite follow it could be a reason to fork personally. Every author has its own preferences, that's doesn't matter for end users, if there are no clear and huge benefits to migrate, I think the author's preferences are just fine.

For dependencies, we of course could and should find more adoptable dependencies. And for is-builtin-module, I don't think it's useful because it actually doesn't check the specific node version, for examine, node: protocol modules are not available on legacy Node versions, of course it could be argued that we don't support legacy Node versions, then why module.isBuiltin won't work? I still don't understand what does this package trying to resolve.

The most important part is, if it's forked, then which one should I choose and why? This is really confusing for users.

[43081j]

The devDependencies are not the reason to fork 👍

Contributions should and will happen upstream, but it seems not all of them will be accepted. So a fork makes sense in that case.

I don't think we should sacrifice performance improvements, even if having two choices of lint plugin is more effort than having one. Choices should be fine, many lint plugins have alternatives already (while the original is maintained).

[fisker]

why module.isBuiltin won't work?

Replace it with module.isBuiltin(), run unicorn against this code

import x from 'inspector/promises'

On Node.js 18 & 20.

There will be more builtin modules added in future, static list makes result consistent.

[fisker]

Anyway, since eslint-plugin-n already have the same rule as we do, we already consider to remove perfer-node-protocol sindresorhus/eslint-plugin-unicorn#1778 (comment)

Currently unicorn version checkes more places than eslint-plugin-n, but once eslint-community/eslint-plugin-n#430 land, they will be the same.

[JounQin]

There will be more builtin modules added in future, static list makes result consistent.

That's why we should prefer using module.isBuiltin, Node 18 users shouldn't be able to consider Node 20 only modules as built-in, because it's just indeed unavailable.

[fisker]

Don't think so, people can lint with 18, run code in 20.

[JounQin]

many lint plugins have alternatives already (while the original is maintained)

Useless there are huge improvements or new features, or if the DX is just terrible on upstream.

I don't see all of them in this case.

---

OK, maybe the is-builtin-module part could be one.