Skip to content
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: export Ux and add configAggregator to SfCommand #106

Merged
merged 7 commits into from
Oct 20, 2022

Conversation

mdonnalley
Copy link
Contributor

@mdonnalley mdonnalley commented Oct 14, 2022

  • Exports Ux class
  • Moves spinner and prompter instances to Ux
  • Adds configAggregator property to SfCommand
    • Defaults to ConfigAggregator unless this.config.bin === 'sfdx', in which case it's a SfdxConfigAggregator

@W-11910294@

@mdonnalley mdonnalley self-assigned this Oct 14, 2022
Copy link
Contributor

@mshanemc mshanemc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ConfigAggregator seems important enough to add to the readme or docs somewhere, especially the odd behavior with trying to figure out your CLI in real life OR running locally and treating it like sf.

Ex: how would you write a NUT test to check that you've correctly implemented sfdx behavior? Or just say you can't?

Next: does the ux/ux class need some typedoc since it's exported now? Maybe some example code for how a Command might pass its jsonEnabled state to a helper that uses this class?

Finally, do we want to help people not use this in a command (linter considerations) to encourage them to use the this.foo for ux? Or would that be fine if they did?

@mshanemc
Copy link
Contributor

mshanemc commented Oct 19, 2022

QA: I have this running locally with plugin-data.

used ConfigAggregator (in the sf flavor)
used the newer flag aliases (I bumped oclif core so I could get that)

Ux:
❓ Use case: I can pass a boolean to a helper class/method OR pass a ux object to it.
I want that thing outside of the command to be able to this.log and this.warn and have it behave correctly. Those methods get inherited from oclif/command, not ux, so these outside-of-a-command still need an additional property and conditional logic everywhere before log/warn/etc.
Screen Shot 2022-10-19 at 7 38 42 AM
[note--warn could be easily accommodated by using the lifecycle stuff from core.

💡 another option would be to expose (readonly) the this.ux.outputEnabled property so that a consumer can easily decide when to log stuff.
currently it's Property 'outputEnabled' is protected and only accessible within class 'UxBase' and its subclasses.

❓ tried to use ux.styledObject. There's some signature difference between
cliUx styledObject(obj: any, keys?: string[]): void;
and the new exported ux: public styledObject(obj: AnyJson): void {

Adding the 2nd property (that array of keys) shouldn't be a breaking change. Or maybe that was an intentional api simplification and you want consumers to filter their own keys? I'm good either way.

❌ the example logic seems backwards. If jsonEnabled() === true, wouldn't that enable output?
Screen Shot 2022-10-19 at 10 08 15 AM

@mshanemc
Copy link
Contributor

QA notes 2:

❓ previous issues are fixed. I tried flag aliases--via bin/dev it worked seamlessly, but I was surprised NOT to get a warning in either stdout or jsonOuput,warnings. Maybe the use case for aliases is broader (convenience, not just backward compatibility) so that makes sense?

✅ passing a constructed ux results in logs and warnings for non-json
❓ passing a constructed ux results in no logs or warnings in the json

@mshanemc mshanemc merged commit 9c03d8d into main Oct 20, 2022
@mshanemc mshanemc deleted the mdonnalley/export-ux branch October 20, 2022 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants