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

fix: sfProject types matches schemas repo #1081

Merged
merged 15 commits into from
Jun 19, 2024

Conversation

mshanemc
Copy link
Contributor

@mshanemc mshanemc commented Jun 7, 2024

What does this PR do?

uses the types from salesforce/schemas for sfProject and its PackageDirs.

This might be a breaking change because consumers might have been using looser types

pino 9 (major version bump) drops support for older node version. Shouldn't affect us.

What issues does this PR fix or reference?

@W-10271718@
@W-5571825@

(!!packageDir.package && packageDir.package === withId.package)
);
});
public addPackageDirectory(packageDir: PackageDir): void {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

NamedPackageDir is a superset of PackageDir. It includes name and fullPath which aren't in PackageDir and shouldn't be written to the project

That still leaves the method in a weird place where you can pass in anything that satisfies packageDir (including namedPackageDir) and its contents [including excess props] get written to the Project, but at least now the method isn't requiring them.

packagingLib was the only place on public github that this was used.

@mshanemc mshanemc mentioned this pull request Jun 17, 2024
@mshanemc mshanemc marked this pull request as ready for review June 18, 2024 14:55
@mshanemc mshanemc requested a review from a team as a code owner June 18, 2024 14:55
Copy link
Member

@WillieRuemmele WillieRuemmele left a comment

Choose a reason for hiding this comment

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

would like clarification around the anticipated release (is it major)

I'd also like more comments around the types, and when they should apply... but since we're probably the only ones using them, maybe it's a moot point 🤷‍♂️

@@ -0,0 +1,15 @@
# Migrating `@salesforce/core` from v7 to v8

v8 uses the types from `@salesforce/schemas`, for SfProject/SfProjectJson.
Copy link
Member

Choose a reason for hiding this comment

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

SfProject/SfPr... using `

The schemas types are more accurate about the PackageDir type (what `sfdx-project.json` packageDirectories is an array of). It is a union type of

1. simply `path`, with an optional default property
1. 1, along with the `package` property and lots of optional properties used for packaging.
Copy link
Member

Choose a reason for hiding this comment

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

should be 2?, also "1. 1,"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't have to be. markdown renders them correctly like this, and the you can change the order without retyping them.

To support differentiating between the two structures, 2 type guards are now importable:

1. isPackagingDirectory
1. isNamedPackagingDirectory
Copy link
Member

Choose a reason for hiding this comment

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

2

@@ -53,9 +53,9 @@
],
"dependencies": {
Copy link
Member

Choose a reason for hiding this comment

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

is this a major version bump? I don't see it in in commits with ! or in the version here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, there's a feat! commit in there with BREAKING CHANGES: etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/sfProject.ts Outdated
plugins?: { [k: string]: unknown };
packageAliases?: { [k: string]: string };
};
export type NamedPackagingDir = PackagePackageDir & NamedDirAdditions;
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between "package" and "packaging" - would be nice to have comments here explaining

NIT: NamedDirAdditions also unreasonably iirks me, maybe just NameAndPath so that it becomes PackagePackageDir & NameAndPath?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in sfdx-project.json they're called packageDirectories even if you're not using them for packaging. Packaging refers to 1gp/2gp packages

I like you idea but will use NameAndFullPath since path is already on the basic one

src/sfProject.ts Show resolved Hide resolved
src/sfProject.ts Show resolved Hide resolved
@WillieRuemmele WillieRuemmele merged commit 750af5c into main Jun 19, 2024
73 checks passed
@WillieRuemmele WillieRuemmele deleted the sm/project-type-consolidation branch June 19, 2024 21:24
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