Skip to content

Conversation

@velzie
Copy link
Contributor

@velzie velzie commented Jan 20, 2026

Shuffles some things around in opts.js - first step towards trying to address #928

First adds a jsdoc typedef to the parsed options struct, which should help maintainability a bit.

To make it more ergonomic, we should move more of the input validation into this function as well. I already moved the defaultExt validation/normalization there, and will probably move more things there or add additional validation later

defaults.json has also been removed, I'm not sure it makes much sense to keep it as a json file since some of the options can be non trivial datatypes like functions, and we'll probably end up having a separate set of cli option defaults that can be somewhere else

Tests are all passing and this doesn't seem like too much of a risky change but could use some review.

Let me know if you have any feedback

@jelveh jelveh requested a review from KernelDeimos January 21, 2026 23:05
@KernelDeimos
Copy link
Contributor

looks good to me. The diff shows --hide-permissions bring added but I think that's because the branch needs a rebase. I'll try to re-run the failing check.

@KernelDeimos
Copy link
Contributor

Tests pass - I'll merge when the diff is clean (rebased onto main)

@velzie
Copy link
Contributor Author

velzie commented Jan 22, 2026

should be clean now

@KernelDeimos
Copy link
Contributor

Great, much easier to verify without the noise. That said, even though I reviewed this carefully don't rely on my review 100% - keep a mental background process running for a day or two to try find any potential regressions with this change because it could significantly impact any automation with shell scripts etc using the commandline. We need to be really careful about this (I might sound repetitive but it can't be emphasized enough) because this package is relatively early in the node ecosystem.

@KernelDeimos KernelDeimos merged commit 741ae2b into http-party:master Jan 22, 2026
9 checks passed
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