-
Notifications
You must be signed in to change notification settings - Fork 73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add strategies for command discovery #945
Conversation
src/config/plugin.ts
Outdated
if (commandDiscovery.strategy === 'explicit') { | ||
if (!commandDiscovery.target) throw new CLIError('`oclif.commandDiscovery.target` is required.') | ||
return commandDiscovery | ||
} | ||
|
||
if (commandDiscovery.strategy === 'pattern') { | ||
if (!commandDiscovery.target) { | ||
throw new CLIError('`oclif.commandDiscovery.target` is required.') | ||
} | ||
|
||
return commandDiscovery | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these separate check/throws for the same condition/error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a vestige of a previous implementation - I'll clean it up
function determineCommandDiscoveryOptions( | ||
commandDiscovery: string | CommandDiscovery | undefined, | ||
): CommandDiscovery | undefined { | ||
if (!commandDiscovery) return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to simplify types and undefined checks further down, what if it always returned a CommandDiscovery (and the return from undefined
sets it to the default behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are instances where there's not any command discovery, i.e. plugins that only have hooks but no commands (e.g. plugin-not-found)
Setting to the default would probably be fine, since it just wouldn't find any commands but then you're incurring the cost of searching for commands when we already know that none will be found
|
||
export default { | ||
'foo:bar': FooBar, | ||
'foo:baz': FooBaz, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know you tested my "2 keys pointing at one file" manually, but what about adding it to the fixture so we can't break that behavior once people start depending on it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approve, one type-only suggestion
Co-authored-by: Shane McLaughlin <shane.mclaughlin@salesforce.com>
QA:
✅ as is (no pjson.oclif changes): works fine
🟡 command executes, but it also displayed this before.
I feel like this is likely an artifact of plugin linking doing tsnode things? ok, let's publish the plugin via npm and manually install it into sf instead of linking. "as is"
❌
|
QA cont
"commands": {
"strategy": "pattern",
"target": "./lib/commands",
"globPatterns": [
"**/index.+(js|cjs|mjs|ts|tsx|mts|cts)"
]
}, ❌ when building with
I think that traces to https://github.com/oclif/plugin-command-snapshot/blob/19a332cfc2773632e469378acc62dab816805cf7/src/commands/schema/generate.ts#L104 which isn't going to be aware of these new globs. published as when installed into a CLI that has this oclif yarn-linked in plugins:link into an |
Review afterthought: have you considered how bundling would work with hooks? As is, I think hooks are defined in pjson with filepaths. ex:
Wouldn't you need the hook to point to something inside the bundled file instead of an entire file? |
QA cont bad target like "hooks": {
"init": [
{
"target": "./dist/bad.js",
"identifier": "hook2"
}
]
} ✅ /❓ runs without error. DEBUG=* is needed to shows the
using
other oclif and sf plugins provide a nicer error
|
with ✅ honors tax-free |
src/config/plugin.ts
Outdated
private async getCommandIdsFromTarget(): Promise<string[] | undefined> { | ||
const commandsFromExport = await this.loadCommandsFromTarget() | ||
if (commandsFromExport) { | ||
return ( | ||
Object.entries(commandsFromExport) | ||
.map(([id, cmd]) => { | ||
if (!ensureCommandClass(cmd)) return | ||
return id | ||
}) | ||
// eslint-disable-next-line unicorn/prefer-native-coercion-functions | ||
.filter((f): f is string => Boolean(f)) | ||
) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you filter first you don't have to deal with undefineds. You can also modify the method to always return an array since the 2 consumers are doing ?? []
after it anyway.
private async getCommandIdsFromTarget(): Promise<string[] | undefined> { | |
const commandsFromExport = await this.loadCommandsFromTarget() | |
if (commandsFromExport) { | |
return ( | |
Object.entries(commandsFromExport) | |
.map(([id, cmd]) => { | |
if (!ensureCommandClass(cmd)) return | |
return id | |
}) | |
// eslint-disable-next-line unicorn/prefer-native-coercion-functions | |
.filter((f): f is string => Boolean(f)) | |
) | |
} | |
} | |
private async getCommandIdsFromTarget(): Promise<string[]> { | |
return Object.entries((await this.loadCommandsFromTarget()) ?? []) | |
.filter(([, cmd]) => ensureCommandClass(cmd)) | |
.map(([id]) => id) | |
} |
QA: plugins-plugins 4.2.6-0.dev into an sf plugins link . in https://github.com/oclif/plugin-test-esbuild
✅ successfully installed plugin-test-esbuild. hellos work, including hooks from dependency @oclif/plugin-test-esm-1 |
QA cont [using only plugin-test-esbuild and its bin/run.js] ❌ ./bin/run.js search , then select any command ✅ ✅
|
QA cont ✅ linking an oclif plugin works fine ➜ plugin-test-esbuild git:(main) ./bin/run.js version --verbose Architecture: Node Version: Plugin Version: OS and Version: Shell: Root Path: |
QA: install https://github.com/oclif/plugin-autocomplete from github ✅ |
This PR introduces the idea of "command discovery strategies". Three strategies are added:
pattern
,explicit
, andsingle
-explicit
is the only net-new strategy being added here.Command Discovery Strategies
Support three strategies for command discovery:
pattern
- this is the current behavior that finds commands based on glob patternsexplicit
- find commands that are exported from a specified file. This is to support developers who wish to bundle their CLI into a single file.single
- CLI contains a single command executed by top-level binpattern
StrategyFor
pattern
, plugins could continue to do this:which will tell oclif to look for commands in that directory (this is skipped if an
oclif.manifest.json
is present)Alternatively, plugins could set this configuration:
And they could even add
globPatterns
to override the glob patterns that oclif uses when searching for command files:explicit
StrategyFor
explicit
, plugins would add a new file (e.g.src/commands.ts
) and then add this configuration to the package.jsonsrc/index.ts
would then need to have an export with the same name as theidentifier
(if not set, it defaults todefault
) that's an object of command names to command classes, e.g.The
explicit
strategy is useful to those who can't rely on file paths because they've bundled their code (oclif/oclif#653) but it can also be used if you simply prefer to be more explicit about your commands instead of relying on oclif "magically" finding commands from the file system.It can also be leveraged to create or modify commands at runtime (e.g. add flags to a command based on an API spec - see
oclif + dynamic commands
section below).Unfortunately, there is no notable performance improvement for development since most of the time is spent auto-transpiling typescript with ts-node
single
StrategyThis strategy is for single command CLIs, i.e. CLIs whose bin invokes a single command.
The current way to achieve this to set this in the package.json
The
default
tells oclif to use.
as the command id when no command is found andcommands
tells oclif where to find the command file. In the example,./dist
will resolve to to./dist/index.js
The
default
property has always been a less-than-ideal workaround and will be deprecated in favor of these settings:Note about
oclif.manifest.json
For all strategies, the
oclif.manifest.json
will be used to load the commands instead of the default behavior of the strategy.oclif
+ BundlingForewarning: we do not plan to support bundling given the endless number of tools + configurations that could be used. But if you choose to use a bundler, like
esbuild
there are a couple hard requirements - you must have a package.json in your root directory and abin/run
orbin/run.js
bin script. This means that you will not be able to successfully bundle your entire CLI (src code, package.json, node_modules, etc) into a single file.If you still want to bundle your CLI or plugin into a single file you will need to do the following (see example repo):
explicit
strategy (see description above). Be sure to set thetarget
to your bundled file and theidentifier
to the name of the object that contains your commandsidentifier
andtarget
.Traditionally, hooks have been defined like this:
But in order for them to work once they been bundled into a single file, you'll need to instead define them like this:
That configuration is essentially telling oclif to look for an
INIT_HOOK
export inside of./dist/index.js
oclif
+ Dynamic CommandsYou can use the
explicit
strategy if you want to manipulate or create commands at runtime. Please note that such usage of theexplicit
strategy cannot be used with anoclif.manifest.json
.Example:
QA
pattern
strategy (defined as string)oclif generate my-cli
rm -rf dist
yarn link @oclif/core
bin/dev.js hello world
pattern
strategy (defined as object)oclif generate my-cli
rm -rf dist
yarn link @oclif/core
oclif.commands
in pjson to:bin/dev.js hello world
world.helpers.ts
tosrc/commands
oclif.commands
in pjson to:bin/dev.js hello world
explicit
strategyoclif generate my-cli
rm -rf dist
yarn link @oclif/core
oclif.commands
in pjson to:src/commands.js
and put this in it:bin/dev.js hello world
bin/dev.js howdy
identifier
from config and changeexport const COMMANDS
toexport default const
bin/dev.js hello world
bin/dev.js howdy
explicit
strategy in bundled pluginyarn
bin/dev.js esbuild
This plugin has an
init
hook so you should see output from that as well as output from the commandSingle command cli using
default
oclif generate my-cli
rm -rf dist
yarn link @oclif/core
src/index.ts
oclif
section of pjson to:bin/dev.js
single
strategyoclif generate my-cli
rm -rf dist
yarn link @oclif/core
src/index.ts
oclif.commands
in pjson to:bin/dev.js
sf
integrationFor this you will need to:
@oclif/core@dev
andoclif@dev
sf plugins install @oclif/plugin-test-esbuild
sf esbuild
Work Items and Issues
Closes #943
Fixes oclif/oclif#653
Fixes oclif/oclif#1053
Fixes #965
Fixes #979
Fixes #995
@W-15005785@