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: merge cli-ux library with oclif/core #345

Merged
merged 28 commits into from
Jan 26, 2022
Merged

Conversation

peternhale
Copy link
Contributor

@peternhale peternhale commented Jan 14, 2022

Removes the circular dependency between oclif/core and cli-ux
@W-10255961@

Removes the circlular dependency between oclif/core and cli-ux
@W-10255961@
Removes the circlular dependency between oclif/core and cli-ux
test/cli-ux/index.test.ts Outdated Show resolved Hide resolved
@@ -41,7 +50,7 @@ describe('Salesforce CLI (sf)', () => {
* ENVIRONMENT VARIABLES
* <environment variables>
*/
const regex = /^[A-Z].*\n\nUSAGE[\S\s]*\n\nFLAGS[\S\s]*\n\nGLOBAL FLAGS[\S\s]*\n\nDESCRIPTION[\S\s]*\n\nEXAMPLES[\S\s]*\n\nFLAG DESCRIPTIONS[\S\s]*\n\nCONFIGURATION VARIABLES[\S\s]*\n\nENVIRONMENT VARIABLES[\S\s]*$/g
const regex = /^.*?USAGE.*?FLAGS.*?GLOBAL FLAGS.*?DESCRIPTION.*?EXAMPLES.*?FLAG DESCRIPTIONS.*?CONFIGURATION VARIABLES.*?ENVIRONMENT VARIABLES.*$/gs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Simplified this a bit

@@ -62,7 +71,7 @@ describe('Salesforce CLI (sf)', () => {
* GLOBAL FLAGS
* <global flags>
*/
const regex = /^[A-Z].*\n\nUSAGE[\S\s]*\n\nFLAGS[\S\s]*\n\nGLOBAL FLAGS[\S\s]*$/g
const regex = /^.*?USAGE.*?FLAGS.*?GLOBAL FLAGS.*?(?!DESCRIPTION).*?(?!EXAMPLES).*?(?!FLAG DESCRIPTIONS).*?(?!CONFIGURATION VARIABLES).*?(?!ENVIRONMENT VARIABLES).*$/gs
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I extended this regex to confirm that the other section present when using --help are not present on -h

test/integration/sf.e2e.ts Outdated Show resolved Hide resolved
const chalk = require('chalk')
chalk.level = 0

function parseJson(json: string) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running tests locally I was getting colored JSON results, so stripping those chars from the response.

src/cli-ux/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
mdonnalley
mdonnalley previously approved these changes Jan 20, 2022
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
```

This extends [cli-progress](https://www.npmjs.com/package/cli-progress)
see all of the options and customizations there, which can be passed in with the options object.
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is "there"? Can you clarify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is the linked reference to cli-progress module.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha. How about changing this sentence to:

"See the cli-progress module for all the options and the customizations that can be passed in with the options object."

(not sure if I got that correct, change as needed.)

Co-authored-by: Juliet Shackell <63259011+jshackell-sfdc@users.noreply.github.com>
src/index.ts Outdated
@@ -42,6 +44,17 @@ export {
flush,
}

export namespace CliUx {
Copy link
Contributor

Choose a reason for hiding this comment

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

In testing, this caused some compilation issues when accessing CliUx.Table and CliUx.Config

I was able to fix it by removing the namespace and adding this to the exports on line 21, cliUx as CliUx

Copy link
Contributor

@mdonnalley mdonnalley left a comment

Choose a reason for hiding this comment

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

@peternhale I'm seeing this on sf retrieve metadata when it tries to instantiate the spinner:

~/repos/trailheadapps/dreamhouse-lwc [main] : ~/repos/salesforcecli/plugin-deploy-retrieve-metadata/bin/dev retrieve metadata --metadata ApexClass
TypeError: (0 , castArray_1.default) is not a function
    at SpinnerAction._write (/Users/mdonnalley/repos/oclif/core/lib/cli-ux/action/base.js:163:76)
    at SpinnerAction._render (/Users/mdonnalley/repos/oclif/core/lib/cli-ux/action/spinner.js:62:14)
    at SpinnerAction._start (/Users/mdonnalley/repos/oclif/core/lib/cli-ux/action/spinner.js:27:14)
    at SpinnerAction.start (/Users/mdonnalley/repos/oclif/core/lib/cli-ux/action/base.js:18:14)
    at /Users/mdonnalley/repos/salesforcecli/sf-plugins-core/src/ux/spinner.ts:24:42
    at Spinner.maybeNoop (/Users/mdonnalley/repos/salesforcecli/sf-plugins-core/src/ux/ux.ts:31:29)
    at Spinner.start (/Users/mdonnalley/repos/salesforcecli/sf-plugins-core/src/ux/spinner.ts:24:10)
    at RetrieveMetadata.run (/Users/mdonnalley/repos/salesforcecli/plugin-deploy-retrieve-metadata/src/commands/retrieve/metadata.ts:140:18)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at RetrieveMetadata._run (/Users/mdonnalley/repos/oclif/core/lib/command.js:52:22)
    at Config.runCommand (/Users/mdonnalley/repos/salesforcecli/plugin-deploy-retrieve-metadata/node_modules/@oclif/core/lib/config/config.js:222:25)
Retrieving Metadata... done

mdonnalley
mdonnalley previously approved these changes Jan 25, 2022
@mdonnalley mdonnalley merged commit 27175d6 into main Jan 26, 2022
@mdonnalley mdonnalley deleted the phale/remove-cycle branch January 26, 2022 15:54
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.

4 participants