CLI: Integrate CLI triage into this package #10131
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10131 +/- ##
==========================================
+ Coverage 85.35% 85.36% +0.01%
==========================================
Files 337 338 +1
Lines 13722 13811 +89
==========================================
+ Hits 11712 11790 +78
- Misses 2010 2021 +11
Continue to review full report at Codecov.
|
medikoo
force-pushed
the
1021-move-triage
branch
2 times, most recently
from
1100733
to
2502ace
Compare
medikoo
force-pushed
the
1021-move-triage
branch
from
2502ace
to
1a10523
Compare
| const componentsCommands = new Set(['registry', 'init', 'publish']); | ||
| if (componentsCommands.has(process.argv[2])) return '@serverless/components'; | ||
| if (cliArgs.has('--help-components')) return '@serverless/components'; | ||
| if (cliArgs.has('--target')) return '@serverless/components'; |
There was a problem hiding this comment.
What if a Framework or Tencent command is named registry, init or publish? Or has a --target option? Especially with plugins that could define these?
There was a problem hiding this comment.
Currently, we unconditionally run Components in such case (note Tencent is Components, as@serveless/components it's about both AWS and Tencent components engines here). --target is Tencent specific thing
There was a problem hiding this comment.
OK, so I guess the logic is: registry, init and publish are official commands. Plugins shouldn't override them.
Based on that statement, as long as we don't implement those commands in the Framework we can keep that if. The day we want to add these commands to the Framework, we can update/remove the if without problem (it's just an optimization).
So the last thing I see that is much less obvious is --target: that option is currently used by Tencent, but nothing forbids a plugin to add support for such an option, correct? It seems it might be safer to remove just this specific logic, right?
There was a problem hiding this comment.
All of this is controversial, but it's the price of attempting to host very different CLI programs under one name.
Currently, we do not forbid plugins from implementing registry, init, publish commands or add --target flag, yet it just won't work if they do, and definitely plugin author will be very confused seeing that.
We may implement some extra validation handling these specific cases (the same way we should probably add validation that forbids re-defining global aliases, as -h and -v), still, I'd create another issue for that, technically it's not in the scope of this PR (which just moves current triage logic into this repository)
There was a problem hiding this comment.
I understand and agree.
What I'm focusing on is that for registry, init and publish, the compromise is reasonable. We already have that compromise with other CLI commands (deploy, package, etc.).
But for --target there is much more room for confusion, and the gain doesn't look obvious here. As such, the ratio gain/risk is lower: it seems that the compromise isn't worth it for that option.
Is there anything specific about that CLI option that I'm missing that makes it really really worth taking the risk?
There was a problem hiding this comment.
@mnapoli do this discussion belongs to this PR? Note this PR doesn't introduce any changes, all the goal is to move existing (as it works currently) triage logic into this repository.
Otherwise I totally agree, that it'll be great to rethink approach to --target, as it's quite dangerous to have such generic option taken over by Tencent completely
There was a problem hiding this comment.
So instead of removing the line here what should we do? Feel free to do a separate PR if you think that's better, I'm just saying we want to fix this, and you seem to agree on that…
There was a problem hiding this comment.
My point is, that any changes to triage, should rather be discussed separately, and I believe we also need consensus from @ole3021 on that.
e.g. now removing handling of --target here, and merging this, will change how triage works, and will change the experience for Tencent users
|
@pgrzesik can you re-review? @zongUMR helped to pick issues in initial version, and now after fixes it's confirmed to reflect current tirage logic. What changed:
|
Addresses #9855
Integrates CLI triage logic found in:
@serverless/cli)@serverless/components)into this package, so no modules of given CLI are loaded unless it's chosen by triage.
It's covered by tests, and while I've ensured to replicate current logic, I've also applied a few optimization shortcuts. Please follow configured logic for details
Performance-wise this cuts down .5-.8s (on my machine) from the startup time of any command. Still, it's too early to open the champagne, as still, startup is slow on
serverlessCLI side (e.g.sls --versionfrom taking 2s, got down to 1.2s, which is still not satisfactory)