fix: do not mutate process.argv

[agilgur5]

• 1.7.1 introduced some arr.splice's into the code, which caused
  mutations in process.argv, affecting downstream code

• set arr to a clone of process.argv instead so it can be freely
  mutated after

• explicitly call the parameter processArgv so code is written more
  carefully when dealing with it

• add a test to ensure process.argv is not mutated

---

Hi @lukeed -- I'm writing this as URGENT as it would appear some code in 1.7.1 was introduced that mutated process.argv, which is causing issues downstream, see jaredpalmer/tsdx#511 (comment) , agilgur5/jest-without-globals#2 (comment) .

Note that the test I added fails on 1.7.1 and 1.7.2. The Usage docs also say to pass process.argv directly to parse (not a clone), so internal usage should be very careful when using it.

Could we get this merged and published ASAP before it affects more code downstream?
It causes some extremely confusing errors and it was very time-consuming for me to figure out what was happening and where it was coming from 😕

[lukeed]

Thanks, I won't be able to return to a computer until tonight. Have you confirmed that this works upstream?

[agilgur5]

Assuming you meant downstream -- in the issues/PRs/comments I linked, I've said that a temporary workaround is to downgrade to 1.7.0. I did that manually in my package-lock.json in agilgur5/jest-without-globals#2 (comment) and it did solve the issue.
I also confirmed in those that the error is due to process.argv mutation and that it only started occurring in the past few days (coinciding with the release of 1.7.1 / 1.7.2).

I only changed one line in the source so that process.argv is cloned first before any further operations are made, making it safe to mutate the clone. I did not attempt to actually remove the splices or dive into that code.
I also added a test here to ensure that the fix works. Per my opening comment, that test fails on 1.7.1 and 1.7.2.

Tried to also stay consistent with your style (tabs, let, and ; in particular), though there isn't a linter used here and that's very low priority compared to fixing this.

[agilgur5]

If you meant confirming the fix downstream, yes, I manually edited my node_modules/sade/lib/index.js to use this same code and that fixed the error.

[lukeed]

Yes I meant local downstream testing. I also followed up on your other references here.
I made light changes, the majority of which was just adding a lot more tests. Even though you added one (thank you 🙏) it didn't actually hit any areas of significance.

Thanks for the investigations & the fix! Sorry for the trouble in the first place 🙈

Also apologies on the slight delay – I am traveling this weekend (of all weekends!)

[agilgur5]

Thanks for getting this merged in quick before it became a much larger issue downstream 🙂

I also followed up on your other references here.

Not sure what you meant by this

it didn't actually hit any areas of significance

I mean, that test failed on 1.7.1 & 1.7.2, so I wouldn't say it hit nothing.

I originally tried to add the test inside the parse :: lazy tests as those are more exhaustive, but all the running multiple times with different parameters made it pretty difficult to change without substantial refactoring.

---

We have very different programming styles and this is your library, but thought I'd throw in my 2¢:

• Removing the comment and processArgv naming removes the intentionality and explicitness behind it.
  Someone else reading the code or you 6 months from now might not know why the array is being copied for some reason, as it's not documented or explicit. Have seen many a case of "don't think there should be an issue removing that..." and things blowing up. The tests will check for that at least, but that's a lot further delayed than a comment.
• I wrote the test as args and argsCopy so that changes to those aren't so brittle and you don't have to change it in a bunch of places. It's also very explicit on the expected side.

Again, just my 2¢

---

I think TSDX accounts for like 1/3 of sade's downloads right now (15k/45k) so might start seeing some more issues from downstream as a result 😅
#27 is something TSDX makes usage of too (@types/sade); I'm not sure how correct those are however, it's never been responded to. Internal types could probably improve on those so that the actual parameters in an action are automatically typed and don't need to be manually typed.