keithamus / sort-package-json

Sort an Object or package.json based on the well-known package.json keys
MIT License
806 stars 87 forks source link

feat: sort all scripts unless `npm-run-all` is a dependency #232

Closed G-Rath closed 3 years ago

G-Rath commented 3 years ago

This restores the default behaviour of sorting scripts alphabetically followed by pre & post unless npm-run-all is a development dependency in the package.json being sorted.

Closes #220

Maxim-Mazurok commented 3 years ago

Doesn't cover npx npm-run-all case, but having npm-run-all as a dev dependency seems like a reasonable requirement in case if I don't want my scripts to be sorted. LGTM 👍

fisker commented 3 years ago

The idea is good, but the implementation is bad, we should not pass packageJson all the way around.

I would check npm-run-all in sortPackageJson function and created a different overFields function.

G-Rath commented 3 years ago

@fisker I'm not sure if I'd say that's a better solution - the advantage I see with passing packageJson around is that it can be used elsewhere for other advanced checks and it's implementation does that in a very clean way by encapsulating the special logic within the function that handles sorting scripts so other functions don't need to know about that.

If you were to make a different overFields function, that'd result in something more complex and special-case-y since it would be devoted purely to the scripts fields, e.g. how would you handle a second independent field with similar behaviour? You'd have a third, maybe even four or more different overFields functions?

That's if I understand your thinking correctly - I've not done a lot of work with this sort of functional currying & piping layout so it's possible I've missed an easy trick :)

G-Rath commented 3 years ago

what do you think my suggestions?

I'm not too opposed to them, but am interested in what others think as I originally did have the code like that, but the problem I saw was that to me it means you don't have this strong guarantee/implication that the parameter is/should-be the package json that was passed in which is part of the underlying idea: that a field is ensured it'll be passed whatever the value of the field is and the whole package json object.

For me this is the sort of thing where I'd feel more comfortable using a general spread if I was writing TypeScript because I know that'll be able to ensure I'm getting passed the right properties for the field function I'm writing but when writing plain Javascript I prefer having that extra distention.

Additionally, this is all internal API - so shipping it how it is right now shouldn't later prevent switching to using a spread if there becomes a reason to sometimes pass more than one argument or an argument that isn't the whole package json object.

tl;dr: I don't know if there's a lot of value in using spread right now when we're always passing in the same two values, vs the extra clarity of using named parameter 🙂

Maxim-Mazurok commented 3 years ago

I don't think that either approach is too bad. I don't think that dragging json is too bad for perf. But maybe better for ease of understanding. Either way, I haven't contributed to this package, so I'm going to let you guys decide. I'm just excited to see #220 fixed asap :)

G-Rath commented 3 years ago

@keithamus are you ok to just commit the suggestions made by @fisker that you want made, since that should be all that's needed but I'm not at my laptop right now 😅

github-actions[bot] commented 3 years ago

:tada: This PR is included in version 1.52.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

bcomnes commented 9 months ago

Posting here too for visibility: You can also add numerics to your script names to force a specific sort order while retaining the ability to automatically alphanumeric sort. IMO this is preferable than adding tool specific behaviors.