Skip to content

Conversation

@mdonnalley
Copy link
Contributor

@mdonnalley mdonnalley commented Apr 23, 2024

What does this PR do?

  • Implement custom logger for @oclif/core to use
  • Simplify CLI startup tasks

Passing release: https://github.com/salesforcecli/cli/actions/runs/9372566389

What issues does this PR fix or reference?

@W-15932535@

@mdonnalley mdonnalley changed the title Mdonnalley/core v4 feat: use sf logger for oclif Apr 23, 2024
@mdonnalley mdonnalley changed the title feat: use sf logger for oclif feat: use oclif/core v4 Jun 5, 2024
bin/run.js Outdated
const pjson = createRequire(import.meta.url)('../package.json');
// Since the CLI is a single process, we can have a larger amount of max listeners since
// the process gets shut down. Don't set it to 0 (no limit) since we should still be aware
// of rouge event listeners
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// of rouge event listeners
// of rogue event listeners

import { type Hook } from '@oclif/core/hooks';
import { toConfiguredId, toStandardizedId } from '@oclif/core/util/ids';
import Interfaces from '@oclif/core/interfaces';
import { loadHelpClass } from '@oclif/core/help';
Copy link
Contributor

Choose a reason for hiding this comment

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

the help barrel isn't small and has a lot of big imports...maybe dynamically load this inside if (argv.includes('--help') || argv.includes('-h')) {


import { Hook, Errors } from '@oclif/core';
import { type Hook } from '@oclif/core/hooks';
import { handle } from '@oclif/core/handle';
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe also only load this if needed inside the conditional instead of when the hook is instantiated?


import { Hook, ux } from '@oclif/core';
import { type Hook } from '@oclif/core/hooks';
import ux from '@oclif/core/ux';
Copy link
Contributor

Choose a reason for hiding this comment

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

also maybe worth deferring to the conditional

package.json Outdated
"debug": "^4.3.4",
"strip-ansi": "^7.1.0"
"@salesforce/sf-plugins-core": "10.0.0",
"ansis": "^3.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

it shipped, you can bump

package.json Outdated
"strip-ansi": "^7.1.0"
"@salesforce/sf-plugins-core": "10.0.0",
"ansis": "^3.2.0",
"debug": "^4.3.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

debug-js/debug#926 you might want the latest of debug for a recent fix

}

await main();
const cli = await import('../dist/cli.js');
Copy link
Contributor

Choose a reason for hiding this comment

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

is it faster to await import? in both these cases we're going to do it anyway so, if not faster, maybe we just use regular imports

@mshanemc
Copy link
Contributor

mshanemc commented Jun 5, 2024

QA with the entire CLI sf update --version 2.45.6-qa.0

sf commands/commands --json
✅ json seems fine
🎨 I don't like the boxed-in table styling, but maybe that's just me

Logger stuff
✅ no unbound class/method issues
✅ DEBUG=* (output to terminal) looks much nicer (consistent formatting, etc)
🎉 all the oclif plugin loading stuff appears in the pino logs!

🕐 perf
local-only oclif command (sf version) is consistently slightly faster than current nightly (5-8%). sf alias list is about the same 🤷🏻 . Maybe the difference in sharing oclif-core version?

@mshanemc mshanemc merged commit 9ab0d4d into main Jun 5, 2024
@mshanemc mshanemc deleted the mdonnalley/core-v4 branch June 5, 2024 22:45
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.

3 participants