Skip to content
This repository has been archived by the owner on Dec 24, 2021. It is now read-only.

registerCommandsIn and Command improvements (require-all, groups, memberName, reloading) #380

Open
pilcrow opened this issue Mar 5, 2021 · 0 comments

Comments

@pilcrow
Copy link

pilcrow commented Mar 5, 2021

registerCommandsIn has a few shortcomings that could be addressed in a backward incompatible major revision, or perhaps in a new call (e.g., registerCommandsInDir) eventually obsoleting registerCommandsIn (hereafter: rCI).

First, and most dangerously, the implementation is much more permissive than the desired semantics, because of require-all. rCI will load all .js and .json files in the whole tree, which is bad day waiting to happen for some poor dev, or a silently compromised server if I can plop a new file into the commands dir which will be loaded next time, unnoticed.

E.g.,

/* cmd1 and cmd2 will be registered under mygrp, but oops and yikes will be silently loaded, too */
.../
    oops.json
    mygrp/
          cmd1.js
          cmd2.js
          subdir/
                 yikes.js

Second, the implementation can effectively be called once only — calling it with a different directory later confounds un/loading, because of the single commandsPath member. If rCI should be called only once, or only on the same directory, that should be enforced and documented.

Third, a command's .group attribute ought to be implied by filesystem location. Right now it's possible to put mycmd.js in a parent directory whose name does not match the Command's group, and this again breaks later un/loading. And if it does match, it's redundant. The fact that Commando insists upon command grouping is a good thing! We should take a page from Rails' "convention over configuration" here and demand it. (Commands registered without a file would have to specify their own .group of course.)

Finally, .memberName, if it is still meaningful after addressing the above, should be implied by the command file's basename. Similar reasons to the above — breaks reloading and is redundant work for the dev.

As for implementation, I think the easiest thing to do is to recurse only two levels deep, infer group from subdirectories, and perhaps record the loaded filename at the time of require. If a command object has .loadedFromFile, it can be reloaded. If not, not.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants