feat: 🎸 custom commands definition in cli #217
Conversation
lukasPan
changed the title
packages/cli/bin/merkur.mjs
Outdated
| // Get merkur custom commands | ||
| const merkurDir = path.resolve(process.cwd(), 'node_modules/@merkur'); | ||
| if (fs.existsSync(merkurDir)) { | ||
| let dirs = []; |
There was a problem hiding this comment.
Why are we setting a default value here and right on the next line we assign a new value? Can't we just assign the new value directly?
There was a problem hiding this comment.
New value assign directly.
packages/cli/bin/merkur.mjs
Outdated
| dirs = fs.readdirSync(merkurDir) | ||
| .map(dir => ({ dir })) | ||
|
|
||
| for (const { dir } of dirs) { |
There was a problem hiding this comment.
This mapping feels a bit odd, you are changeing the structure from dirs = ['dir1', 'dir2'] to dirs = [{ dir: 'dir1' }, { dir: 'dir2' }] and right below this line you are again parsing the dir key from each object. What is the point of changing this structure in the first place?
| dirs = fs.readdirSync(merkurDir) | |
| .map(dir => ({ dir })) | |
| for (const { dir } of dirs) { | |
| dirs = fs.readdirSync(merkurDir); | |
| for (const dir of dirs) { |
There was a problem hiding this comment.
We don't need to change the structure. Removing dir key.
packages/cli/bin/merkur.mjs
Outdated
| fs.readdirSync(commandsDir) | ||
| .map(command => ({ dir: commandsDir, command })) | ||
| .filter( | ||
| ({ command, dir }) => !customCommands.some(({ command: existingCommand }) => existingCommand === command) && |
There was a problem hiding this comment.
I feel like we should print at least a warning, that we are ignoring some commands due to using the same file name. Or throw an error. Or just try to load everything and hope for the best... but I am not sure what would commander do if you try to load a command with the same name twice... maybe it would throw an error so we don't really have to deal with this at all and really just load everything?
Right now, if we have
@merkur/my-plugin-1
- lib
- commands
- my-command.js
@merkur/my-plugin-2
- lib
- commands
- my-command.js
In this case, only the command from @merkur/my-plugin-1 will be loaded and @merkur/my-plugin-2 will be silently ignored, which can cause some confusion.
packages/cli/bin/merkur.mjs
Outdated
| .filter( | ||
| ({ command, dir }) => !customCommands.some(({ command: existingCommand }) => existingCommand === command) && | ||
| fs.statSync(path.join(dir, command)).isFile() && | ||
| (command.endsWith('.js') || command.endsWith('.mjs')) |
There was a problem hiding this comment.
Maybe we should support also .cjs format, since we already support 2 out of 3 here?
There was a problem hiding this comment.
.cjs format added
|
It's a nice feature, but will it just reduce the command, but there is really no connection with Merkur? I think that if it has access to Merkur configuration then it will be much more super drupper feature e.g. as its first argument. |
No description provided.