fix(command): don't mutate stored options when registering#10715
fix(command): don't mutate stored options when registering#10715kyungseopk1m wants to merge 3 commits into
Conversation
register() used args.shift() on the stored option arrays, so registering the same command instance more than once (e.g. when firebase-tools is imported as a module across multiple invocations) lost the flags and threw "Cannot read properties of undefined (reading 'indexOf')". Use destructuring so the stored options aren't modified. Fixes firebase#9929
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where registering a command multiple times mutated its stored options array by shifting the flags out of it. This was resolved by destructuring the options array in src/command.ts instead of using shift(). A corresponding unit test was added in src/command.spec.ts. The review feedback recommends avoiding the use of as unknown type assertions in the new test file to comply with the repository style guide, suggesting proper mock definitions and using Reflect.get to access private properties instead.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10715 +/- ##
=======================================
Coverage ? 57.88%
=======================================
Files ? 609
Lines ? 39226
Branches ? 7867
=======================================
Hits ? 22707
Misses ? 14693
Partials ? 1826 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4 participants
register()usedargs.shift(), which mutated the stored option arrays in place. Registering the same command instance a second time (e.g. when firebase-tools is imported as a module across multiple invocations) then passedundefinedto commander and threwCannot read properties of undefined (reading 'indexOf').Switched to destructuring so the stored options aren't modified, and added a regression test that registers a command multiple times.
This is the small fix that was split out of #9935.
Fixes #9929