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
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions .github/workflows/perf.yml
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ jobs:
steps:
- uses: actions/checkout@v4
- uses: actions/setup-node@v4
with:
cache: yarn
- uses: salesforcecli/github-workflows/.github/actions/yarnInstallWithRetries@main
with:
ignore-scripts: true
Expand Down
5,289 changes: 1,315 additions & 3,974 deletions CHANGELOG.md

Large diffs are not rendered by default.

15 changes: 15 additions & 0 deletions MIGRATING_V7-V8.md
Original file line number Diff line number Diff line change
@@ -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


`PackageDir` and `PackageDirDependency` are no longer re-exported. If you need them, import them from `@salesforce/schemas`
12 changes: 6 additions & 6 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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.

"@jsforce/jsforce-node": "^3.2.0",
"@salesforce/kit": "^3.1.2",
"@salesforce/kit": "^3.1.6",
"@salesforce/schemas": "^1.9.0",
"@salesforce/ts-types": "^2.0.9",
"@salesforce/ts-types": "^2.0.10",
"ajv": "^8.15.0",
"change-case": "^4.1.2",
"fast-levenshtein": "^3.0.0",
Expand All @@ -64,16 +64,16 @@
"js2xmlparser": "^4.0.1",
"jsonwebtoken": "9.0.2",
"jszip": "3.10.1",
"pino": "^8.21.0",
"pino": "^9.2.0",
"pino-abstract-transport": "^1.2.0",
"pino-pretty": "^10.3.1",
"pino-pretty": "^11.2.1",
"proper-lockfile": "^4.1.2",
"semver": "^7.6.2",
"ts-retry-promise": "^0.8.1"
},
"devDependencies": {
"@salesforce/dev-scripts": "^8.5.0",
"@salesforce/ts-sinon": "^1.4.19",
"@salesforce/dev-scripts": "^10.1.1",
"@salesforce/ts-sinon": "^1.4.22",
"@types/benchmark": "^2.1.5",
"@types/chai-string": "^1.4.5",
"@types/fast-levenshtein": "^0.0.4",
Expand Down
2 changes: 1 addition & 1 deletion src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export {

export { OrgConfigProperties, ORG_CONFIG_ALLOWED_PROPERTIES } from './org/orgConfigProperties';

export { PackageDir, NamedPackageDir, PackageDirDependency, SfProject, SfProjectJson } from './sfProject';
export { NamedPackageDir, SfProject, SfProjectJson } from './sfProject';

export { SchemaValidator } from './schema/validator';

Expand Down
3 changes: 2 additions & 1 deletion src/org/scratchOrgInfoGenerator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
import { promises as fs } from 'node:fs';
import { parseJson } from '@salesforce/kit';
import { ensureString } from '@salesforce/ts-types';
import { SfProjectJson } from '../sfProject';
import { SfProjectJson, isPackagingDirectory } from '../sfProject';
import { WebOAuthServer } from '../webOAuthServer';
import { Messages } from '../messages';
import { SfError } from '../sfError';
Expand Down Expand Up @@ -90,6 +90,7 @@ export const getAncestorIds = async (
throw new SfError(messages.getMessage('Package2AncestorsIdsKeyNotSupportedError'), 'DeprecationError');
}
const packagesWithAncestors = (await projectJson.getPackageDirectories())
.filter(isPackagingDirectory)
// check that the package has any ancestor types (id or version)
.filter((packageDir) => packageDir.ancestorId ?? packageDir.ancestorVersion);
if (packagesWithAncestors.length === 0) {
Expand Down
102 changes: 38 additions & 64 deletions src/sfProject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { basename, dirname, isAbsolute, normalize, resolve, sep } from 'node:pat
import * as fs from 'node:fs';
import { defaults, env } from '@salesforce/kit';
import { Dictionary, ensure, JsonMap, Nullable, Optional } from '@salesforce/ts-types';
import { PackageDir, ProjectJson as ProjectJsonSchema, PackagePackageDir } from '@salesforce/schemas';
import { SfdcUrl } from './util/sfdcUrl';
import { ConfigAggregator } from './config/configAggregator';
import { ConfigFile } from './config/configFile';
Expand All @@ -18,45 +19,12 @@ import { resolveProjectPath, resolveProjectPathSync, SFDX_PROJECT_JSON } from '.

import { SfError } from './sfError';
import { Messages } from './messages';
import { findUpperCaseKeys } from './util/findUppercaseKeys';
import { ensureNoUppercaseKeys } from './util/findUppercaseKeys';

Messages.importMessagesDirectory(__dirname);
const messages = Messages.loadMessages('@salesforce/core', 'config');

const coreMessages = Messages.loadMessages('@salesforce/core', 'core');

export type PackageDirDependency = {
[k: string]: unknown;
package: string;
versionNumber?: string;
};

export type PackageDir = {
ancestorId?: string;
ancestorVersion?: string;
default?: boolean;
definitionFile?: string;
dependencies?: PackageDirDependency[];
includeProfileUserLicenses?: boolean;
package?: string;
packageMetadataAccess?: {
permissionSets: string | string[];
permissionSetLicenses: string | string[];
};
path: string;
postInstallScript?: string;
postInstallUrl?: string;
releaseNotesUrl?: string;
scopeProfiles?: boolean;
uninstallScript?: string;
versionDescription?: string;
versionName?: string;
versionNumber?: string;
unpackagedMetadata?: { path: string };
seedMetadata?: { path: string };
};

export type NamedPackageDir = PackageDir & {
type NamedDirAdditions = {
/**
* The [normalized](https://nodejs.org/api/path.html#path_path_normalize_path) path used as the package name.
*/
Expand All @@ -67,16 +35,10 @@ export type NamedPackageDir = PackageDir & {
fullPath: string;
};

export type ProjectJson = ConfigContents & {
packageDirectories: PackageDir[];
namespace?: string;
sourceApiVersion?: string;
sfdcLoginUrl?: string;
signupTargetLoginUrl?: string;
oauthLocalPort?: number;
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

export type NamedPackageDir = PackageDir & NamedDirAdditions;

export type ProjectJson = ConfigContents & ProjectJsonSchema;

/**
* The sfdx-project.json config object. This file determines if a folder is a valid sfdx project.
Expand All @@ -96,6 +58,7 @@ export type ProjectJson = ConfigContents & {
* **See** [force:project:create](https://developer.salesforce.com/docs/atlas.en-us.sfdx_dev.meta/sfdx_dev/sfdx_dev_ws_create_new.htm)
*/
export class SfProjectJson extends ConfigFile<ConfigFile.Options, ProjectJson> {
/** json properties that are uppercase, or allow uppercase keys inside them */
public static BLOCKLIST = ['packageAliases'];

public static getFileName(): string {
Expand Down Expand Up @@ -337,19 +300,9 @@ export class SfProjectJson extends ConfigFile<ConfigFile.Options, ProjectJson> {
*
* @param packageDir
*/
public addPackageDirectory(packageDir: NamedPackageDir): void {
// there is no notion of uniqueness in package directory entries
// so an attempt of matching an existing entry is a bit convoluted
// an entry w/o a package or id is considered a directory entry for which a package has yet to be created
// so first attempt is to find a matching dir entry that where path is the same and id and package are not present
// if that fails, then find a matching dir entry package is present and is same as the new entry
const dirIndex = this.getContents().packageDirectories.findIndex((pd) => {
const withId = pd as NamedPackageDir & { id: string };
return (
(withId.path === packageDir.path && !withId.id && !withId.package) ||
(!!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.

const dirIndex = this.getContents().packageDirectories.findIndex(findPackageDir(packageDir));

// merge new package dir with existing entry, if present
const packageDirEntry: PackageDir = Object.assign(
{},
Expand All @@ -367,17 +320,14 @@ export class SfProjectJson extends ConfigFile<ConfigFile.Options, ProjectJson> {
this.set('packageDirectories', modifiedPackagesDirs);
}

// keep it because testSetup stubs it!
// eslint-disable-next-line class-methods-use-this
private doesPackageExist(packagePath: string): boolean {
return fs.existsSync(packagePath);
}

private validateKeys(): void {
// Verify that the configObject does not have upper case keys; throw if it does. Must be heads down camel case.
const upperCaseKey = findUpperCaseKeys(this.toObject(), SfProjectJson.BLOCKLIST);
if (upperCaseKey) {
throw coreMessages.createError('invalidJsonCasing', [upperCaseKey, this.getPath()]);
}
ensureNoUppercaseKeys(this.getPath())(SfProjectJson.BLOCKLIST)(this.toObject());
}
}

Expand Down Expand Up @@ -600,7 +550,8 @@ export class SfProject {
*/
public getPackageNameFromPath(path: string): Optional<string> {
const packageDir = this.getPackageFromPath(path);
return packageDir ? packageDir.package ?? packageDir.path : undefined;
if (!packageDir) return undefined;
return isNamedPackagingDirectory(packageDir) ? packageDir.package : packageDir.path;
}

/**
Expand Down Expand Up @@ -762,3 +713,26 @@ export class SfProject {
.map(([key]) => key);
}
}

/** differentiate between the Base PackageDir (path, maybe default) and the Packaging version (package and maybe a LOT of other fields) by whether is has the `package` property */
export const isPackagingDirectory = (packageDir: PackageDir): packageDir is PackagePackageDir =>
mshanemc marked this conversation as resolved.
Show resolved Hide resolved
isPackagingDir(packageDir);

/** differentiate between the Base PackageDir (path, maybe default) and the Packaging version (package and maybe a LOT of other fields) by whether is has the `package` property */
export const isNamedPackagingDirectory = (packageDir: NamedPackageDir): packageDir is NamedPackagingDir =>
isPackagingDir(packageDir);

const isPackagingDir = (packageDir: PackageDir | NamedPackageDir): boolean =>
'package' in packageDir && typeof packageDir.package === 'string';
mshanemc marked this conversation as resolved.
Show resolved Hide resolved
/**
* there is no notion of uniqueness in package directory entries
* so an attempt of matching an existing entry is a bit convoluted
*/
const findPackageDir =
(target: PackageDir) =>
(potentialMatch: PackageDir): boolean =>
// an entry w/o a package or id is considered a directory entry for which a package has yet to be created
// find a matching dir entry that where path is the same and id and package are not present
(potentialMatch.path === target.path && !('id' in potentialMatch) && !isPackagingDirectory(potentialMatch)) ||
// if that fails, then find a matching dir entry package is present and is same as the new entry
(isPackagingDirectory(target) && isPackagingDirectory(potentialMatch) && target.package === potentialMatch.package);
38 changes: 21 additions & 17 deletions src/util/findUppercaseKeys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,25 @@
* For full license text, see LICENSE.txt file in the repo root or https://opensource.org/licenses/BSD-3-Clause
*/

import { JsonMap, Optional, isJsonMap, asJsonMap, AnyJson } from '@salesforce/ts-types';
import { findKey } from '@salesforce/kit';
import { strictEqual } from 'node:assert/strict';
import { JsonMap, isJsonMap } from '@salesforce/ts-types';
import { Messages } from '../messages';

export const findUpperCaseKeys = (data?: JsonMap, sectionBlocklist: string[] = []): Optional<string> => {
let key: Optional<string>;
findKey(data, (val: AnyJson, k: string) => {
if (/^[A-Z]/.test(k)) {
key = k;
} else if (isJsonMap(val)) {
if (sectionBlocklist.includes(k)) {
return key;
}
key = findUpperCaseKeys(asJsonMap(val));
}
return key;
});
return key;
};
Messages.importMessagesDirectory(__dirname);
const coreMessages = Messages.loadMessages('@salesforce/core', 'core');

/** will throw on any upperCase unless they are present in the allowList. Recursively searches the object, returning valid keys */
export const ensureNoUppercaseKeys =
(path: string) =>
(allowList: string[] = []) =>
(data: JsonMap): string[] => {
const keys = getKeys(data, allowList);
const upperCaseKeys = keys.filter((key) => /^[A-Z]/.test(key)).join(', ');
strictEqual(upperCaseKeys.length, 0, coreMessages.getMessage('invalidJsonCasing', [upperCaseKeys, path]));
return keys;
};

const getKeys = (data: JsonMap, allowList: string[]): string[] =>
Object.entries(data)
.filter(([k]) => !allowList.includes(k))
.flatMap(([key, value]) => (isJsonMap(value) ? [key, ...getKeys(value, allowList)] : [key]));
Loading
Loading