Refactor index.js

[Niryo]

Are we intentionally keeping this god index.js file as is, or are there plans for breaking it down to more manageable pieces?
I am interested in contributing more stuff to this lib, and I thought that breaking up this huge file could be a good start if there are no objections...

[shadowspawn]

My main concern with splitting up the monolithic index.js is if there are any common client use cases that would be broken by changing to multiple files. (No?)

A minor downside is refactoring the file makes it a harder to track back through the history to find why certain lines of code and logic were introduced. But the benefits to future work outweigh this.

For interest, the file was split up in #661 in a branch to modernise the codebase for v3.

[Niryo]

About the first concern- I am not sure I can't think of a use case where client could be broken because of such an internal change, as long as the api stays the same. The api is the contract, everything else can be changed. If someone is doing some custom scripting based on anything else other than api, it's not commander.js responsibility if it's broken.

About the downside- yep, this is an annoying limitation of git, but as you said, the benefits to future work outweigh the this.
And if you really need to track back through the history, you can still do it, nothing is lost, it just became less convenient.

For me it was super annoying to orient myself inside this huge index.js file, and a split is a no-brainer decision.

[shadowspawn]

I am having a play around with refactoring. Naively splitting Help and Command introduced a circular dependency, but @cravler supplied a work-around with subclass to break the loop. One class per file.

https://github.com/tj/commander.js/tree/feature/refactor-experiment

As of 17 Apr the extracted command.js is 1655 lines, compared with the single file index.js of 2325 lines.

[shadowspawn]

Non-binding feedback, should Commander split up index.js like in refactor-experiment branch? 👍 👎

@abetomo @cravler @Niryo , and any other readers welcome to add feedback too!

[shadowspawn]

I'll have another go at this when Pull Requests are quiet.

[Niryo]

few notes:

1. I am not sure what are the implications of cyclic dependencies here, maybe when can live with it if we can't find any workarounds. But my hunch is that we can probably find a way to structure things in such a way that remove the cyclic dependency.
2. Even if keep those classes in the same file, we will still benefit from extracting small classes, even minor ones.
3. We can also extract pure functions. currently we have many big methods that sits inside the classes themselves, and some of them can be transformed to pure functions sittings outside the classes (where it makes sense of course, not all methods should be extracted this way).

[shadowspawn]

Released in Commander v8.