-
Notifications
You must be signed in to change notification settings - Fork 19
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
plugin-source ESM #992
plugin-source ESM #992
Conversation
This issue has been linked to a new work item: W-14406731 |
2744d96
to
1293abd
Compare
.lintstagedrc.cjs
Outdated
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.
this file can be removed, see: salesforcecli/plugin-login#650 (comment)
@@ -113,7 +115,7 @@ export default class Pull extends SourceCommand { | |||
if (this.flags['api-version']) { | |||
componentSet.apiVersion = this.flags['api-version']; | |||
} | |||
const username = this.flags['target-org'].getUsername(); | |||
const username = this.flags['target-org'].getUsername() as string; |
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 wonder why getUsername
says this is optional when the username is always expected to be available: https://github.com/forcedotcom/sfdx-core/blob/b3c1d6b38aed89b063858352a0b9bc845e814a0f/src/org/connection.ts#L77
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.
The sfdx-core Org
class allows you to instantiate an Org (the class), assign various props, and then write it for the first time.
I've always wanted to make either an option on Org.create to say, "read this from the FS and throw" and then have better typing on that version. Or to have a method that verifies it's coming from the FS and then you can depend on all the expected props being there.
It's kind of hard, though, with a bunch of these being define as class props rather than a type-able object.
@@ -223,6 +227,8 @@ export class DeployResultFormatter extends ResultFormatter { | |||
this.ux.log(''); | |||
this.ux.styledHeader(chalk.red(`Component Failures [${failures.length}]`)); | |||
this.ux.table( | |||
// eslint-disable-next-line @typescript-eslint/ban-ts-comment | |||
// @ts-ignore | |||
failures.map((entry: FileResponseFailure) => ({ |
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 remove the FileResponseFailure
type it shows almost all of the props it tries to access doesn't exist. Are the types from SDR wrong or where does it get corrected?
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 couldn't get this to work without refactoring a bunch of stuff, will verify on QA.
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.
approved with some questions
QA notes: ✅ mdapi/source deploy/retrieve commands, human/json output |
What does this PR do?
ESM'ifies plugin-source
What issues does this PR fix or reference?
@W-14406731@