Skip to content

Commit

Permalink
feat!: remove parameters on config.write
Browse files Browse the repository at this point in the history
The Config family of classes' write(sync) method used to accept a param.
The value in the param would overwrite the existing file.
  • Loading branch information
mshanemc committed Oct 11, 2023
1 parent e7e23bb commit 8e346ca
Show file tree
Hide file tree
Showing 10 changed files with 38 additions and 75 deletions.
6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
{
"name": "@salesforce/core",
"version": "5.3.4",
"version": "6.0.0",
"description": "Core libraries to interact with SFDX projects, orgs, and APIs.",
"main": "lib/exported",
"types": "lib/exported.d.ts",
"license": "BSD-3-Clause",
"engines": {
"node": ">=16.0.0"
"node": ">=18.0.0"
},
"scripts": {
"build": "wireit",
Expand Down Expand Up @@ -48,7 +48,7 @@
"faye": "^1.4.0",
"form-data": "^4.0.0",
"js2xmlparser": "^4.0.1",
"jsforce": "^2.0.0-beta.27",
"jsforce": "^2.0.0-beta.28",
"jsonwebtoken": "9.0.2",
"jszip": "3.10.1",
"pino": "^8.15.1",
Expand Down
6 changes: 1 addition & 5 deletions src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -446,11 +446,7 @@ export class Config extends ConfigFile<ConfigFile.Options, ConfigProperties> {
*
* @param newContents The new Config value to persist.
*/
public async write(newContents?: ConfigProperties): Promise<ConfigProperties> {
if (newContents != null) {
this.setContents(newContents);
}

public async write(): Promise<ConfigProperties> {
await this.cryptProperties(true);

await super.write();
Expand Down
17 changes: 2 additions & 15 deletions src/config/configFile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import { constants as fsConstants, Stats as fsStats } from 'fs';
import { homedir as osHomedir } from 'os';
import { dirname as pathDirname, join as pathJoin } from 'path';
import { lock, lockSync } from 'proper-lockfile';
import { isPlainObject } from '@salesforce/ts-types';
import { parseJsonMap } from '@salesforce/kit';
import { Global } from '../global';
import { Logger } from '../logger/logger';
Expand Down Expand Up @@ -234,7 +233,7 @@ export class ConfigFile<
*
* @param newContents The new contents of the file.
*/
public async write(newContents?: P): Promise<P> {
public async write(): Promise<P> {
// make sure we can write to the directory
try {
await fs.promises.mkdir(pathDirname(this.getPath()), { recursive: true });
Expand All @@ -247,10 +246,6 @@ export class ConfigFile<
// get the file modstamp. Do this after the lock acquisition in case the file is being written to.
const fileTimestamp = (await fs.promises.stat(this.getPath(), { bigint: true })).mtimeNs;

if (isPlainObject(newContents)) {
this.setContents(newContents);
}

// read the file contents into a LWWMap using the modstamp
const stateFromFile = stateFromContents<P>(
parseJsonMap<P>(await fs.promises.readFile(this.getPath(), 'utf8'), this.getPath()),
Expand All @@ -276,26 +271,18 @@ export class ConfigFile<
*
* @param newContents The new contents of the file.
*/
public writeSync(newContents?: P): P {
public writeSync(): P {
try {
fs.mkdirSync(pathDirname(this.getPath()), { recursive: true });
} catch (err) {
throw SfError.wrap(err as Error);
}

if (isPlainObject(newContents)) {
this.setContents(newContents);
}

// lock the file. Returns an unlock function to call when done.
const unlockFn = lockSync(this.getPath(), lockOptions);
// get the file modstamp. Do this after the lock acquisition in case the file is being written to.
const fileTimestamp = fs.statSync(this.getPath(), { bigint: true }).mtimeNs;

if (isPlainObject(newContents)) {
this.setContents(newContents);
}

// read the file contents into a LWWMap using the modstamp
const stateFromFile = stateFromContents<P>(
parseJsonMap<P>(fs.readFileSync(this.getPath(), 'utf8'), this.getPath()),
Expand Down
5 changes: 4 additions & 1 deletion src/config/ttlConfig.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ export class TTLConfig<T extends TTLConfig.Options, P extends JsonMap> extends C
protected async init(): Promise<void> {
const contents = await this.read(this.options.throwOnNotFound);
const date = new Date().getTime();
this.setContents(Object.fromEntries(Object.entries(contents).filter(([, value]) => !this.isExpired(date, value))));
// delete all the expired entries
Object.entries(contents)
.filter(([, value]) => !this.isExpired(date, value))
.map(([key]) => this.unset(key));
}

// eslint-disable-next-line class-methods-use-this
Expand Down
19 changes: 4 additions & 15 deletions src/sfProject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,18 +113,12 @@ export class SfProjectJson extends ConfigFile {
return contents;
}

public async write(newContents?: ConfigContents): Promise<ConfigContents> {
if (newContents) {
this.setContents(newContents);
}
public async write(): Promise<ConfigContents> {
this.validateKeys();
return super.write();
}

public writeSync(newContents?: ConfigContents): ConfigContents {
if (newContents) {
this.setContents(newContents);
}
public writeSync(): ConfigContents {
this.validateKeys();
return super.writeSync();
}
Expand Down Expand Up @@ -334,13 +328,8 @@ export class SfProjectJson extends ConfigFile {
if (!/^.{15,18}$/.test(id)) {
throw messages.createError('invalidId', [id]);
}

const contents = this.getContents();
if (!contents.packageAliases) {
contents.packageAliases = {};
}
contents.packageAliases[alias] = id;
this.setContents(contents);
const newAliases = { ...(this.getContents().packageAliases ?? {}), [alias]: id };
this.contents.set('packageAliases', newAliases);
}

/**
Expand Down
33 changes: 9 additions & 24 deletions src/testSetup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,6 @@ export interface ConfigStub {
* A function to conditionally read based on the config instance. The `this` value will be the config instance.
*/
retrieveContents?: () => Promise<JsonMap>;
/**
* A function to conditionally set based on the config instance. The `this` value will be the config instance.
*/
updateContents?: () => Promise<JsonMap>;
}

/**
Expand Down Expand Up @@ -533,7 +529,7 @@ export const stubContext = (testContext: TestContext): Record<string, SinonStub>

const readSync = function (this: ConfigFile<ConfigFile.Options>, newContents?: JsonMap): JsonMap {
const stub = initStubForRead(this);
this.setContentsFromObject(newContents ?? stub.contents ?? {});
this.setContentsFromFileContents(newContents ?? stub.contents ?? {}, BigInt(Date.now()));
return this.getContents();
};

Expand All @@ -556,33 +552,22 @@ export const stubContext = (testContext: TestContext): Record<string, SinonStub>
// @ts-expect-error: muting exact type match for stub readSync
stubs.configReadSync = testContext.SANDBOXES.CONFIG.stub(ConfigFile.prototype, 'readSync').callsFake(readSync);

const writeSync = function (this: ConfigFile<ConfigFile.Options>, newContents?: ConfigContents): void {
if (!testContext.configStubs[this.constructor.name]) {
testContext.configStubs[this.constructor.name] = {};
}
const stub = testContext.configStubs[this.constructor.name];
if (!stub) return;
const writeSync = function (this: ConfigFile<ConfigFile.Options>): void {
testContext.configStubs[this.constructor.name] ??= {};
const stub = testContext.configStubs[this.constructor.name] ?? {};

this.setContents(newContents ?? this.getContents());
stub.contents = this.toObject();
};

const write = async function (this: ConfigFile<ConfigFile.Options>, newContents?: ConfigContents): Promise<void> {
if (!testContext.configStubs[this.constructor.name]) {
testContext.configStubs[this.constructor.name] = {};
}
const stub = testContext.configStubs[this.constructor.name];
if (!stub) return;
const write = async function (this: ConfigFile<ConfigFile.Options>): Promise<void> {
testContext.configStubs[this.constructor.name] ??= {};
const stub = testContext.configStubs[this.constructor.name] ?? {};

if (stub.writeFn) {
return stub.writeFn.call(this, newContents);
return stub.writeFn.call(this);
}

if (stub.updateContents) {
writeSync.call(this, await stub.updateContents.call(this));
} else {
writeSync.call(this);
}
writeSync.call(this);
};

stubs.configWriteSync = stubMethod(testContext.SANDBOXES.CONFIG, ConfigFile.prototype, 'writeSync').callsFake(
Expand Down
14 changes: 8 additions & 6 deletions test/unit/config/configFileTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ describe('Config', () => {
it('uses passed in contents', async () => {
$$.SANDBOX.stub(fs.promises, 'readFile').resolves('{}');
// @ts-expect-error --> we're only mocking on prop of many
$$.SANDBOX.stub(fs.promises, 'stat').resolves({ mtimeNs: BigInt(new Date().valueOf() - 1_000 * 60 * 5) });
$$.SANDBOX.stub(fs.promises, 'stat').resolves({ mtimeNs: BigInt(Date.now() - 1_000 * 60 * 5) });
$$.SANDBOX.stub(fs.promises, 'mkdir').resolves();
const lockStub = $$.SANDBOX.stub(lockfileLib, 'lock').resolves(() => Promise.resolve());

Expand All @@ -217,7 +217,8 @@ describe('Config', () => {
const config = await TestConfig.create({ isGlobal: true });

const expected = { test: 'test' };
const actual = await config.write(expected);
config.set('test', 'test');
const actual = await config.write();
expect(lockStub.callCount).to.equal(1);
expect(expected).to.deep.equal(actual);
expect(expected).to.deep.equal(config.getContents());
Expand All @@ -227,7 +228,7 @@ describe('Config', () => {
it('sync uses passed in contents', async () => {
$$.SANDBOX.stub(fs, 'readFileSync').returns('{}');
// @ts-expect-error --> we're only mocking on prop of many
$$.SANDBOX.stub(fs, 'statSync').returns({ mtimeNs: BigInt(new Date().valueOf() - 1_000 * 60 * 5) });
$$.SANDBOX.stub(fs, 'statSync').returns({ mtimeNs: BigInt(Date.now() - 1_000 * 60 * 5) });
const lockStub = $$.SANDBOX.stub(lockfileLib, 'lockSync').returns(() => undefined);

const mkdirpStub = $$.SANDBOX.stub(fs, 'mkdirSync');
Expand All @@ -236,7 +237,8 @@ describe('Config', () => {
const config = await TestConfig.create({ isGlobal: true });

const expected = { test: 'test' };
const actual = config.writeSync(expected);
config.set('test', 'test');
const actual = config.writeSync();
expect(lockStub.callCount).to.equal(1);
expect(expected).to.deep.equal(actual);
expect(expected).to.deep.equal(config.getContents());
Expand All @@ -258,7 +260,7 @@ describe('Config', () => {
$$.SANDBOXES.CONFIG.restore();
readFileStub = $$.SANDBOX.stub(fs.promises, 'readFile');
// @ts-expect-error --> we're only mocking on prop of many
$$.SANDBOX.stub(fs.promises, 'stat').resolves({ mtimeNs: BigInt(new Date().valueOf() - 1_000 * 60 * 5) });
$$.SANDBOX.stub(fs.promises, 'stat').resolves({ mtimeNs: BigInt(Date.now() - 1_000 * 60 * 5) });
});

it('caches file contents', async () => {
Expand Down Expand Up @@ -342,7 +344,7 @@ describe('Config', () => {
$$.SANDBOXES.CONFIG.restore();
readFileStub = $$.SANDBOX.stub(fs, 'readFileSync');
// @ts-expect-error --> we're only mocking on prop of many
$$.SANDBOX.stub(fs, 'statSync').returns({ mtimeNs: BigInt(new Date().valueOf() - 1_000 * 60 * 5) });
$$.SANDBOX.stub(fs, 'statSync').returns({ mtimeNs: BigInt(Date.now() - 1_000 * 60 * 5) });
});

it('caches file contents', () => {
Expand Down
2 changes: 1 addition & 1 deletion test/unit/config/configTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ describe('Config', () => {
stubMethod($$.SANDBOX, ConfigFile.prototype, ConfigFile.prototype.read.name).callsFake(async function () {
// @ts-expect-error -> this is any
// eslint-disable-next-line @typescript-eslint/no-unsafe-call
this.setContentsFromObject({ unknown: 'unknown config key and value' });
this.setContentsFromFileContents({ unknown: 'unknown config key and value' }, BigInt(Date.now()));
});

const config = await Config.create({ isGlobal: true });
Expand Down
3 changes: 2 additions & 1 deletion test/unit/projectTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ describe('SfProject', () => {
describe('json', () => {
it('allows uppercase packaging aliases on write', async () => {
const json = await SfProjectJson.create();
await json.write({ packageAliases: { MyName: 'somePackage' } });
json.set('packageAliases', { MyName: 'somePackage' });
await json.write();
// @ts-expect-error possibly undefined
expect($$.getConfigStubContents('SfProjectJson').packageAliases['MyName']).to.equal('somePackage');
});
Expand Down
8 changes: 4 additions & 4 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -3080,10 +3080,10 @@ jsesc@^2.5.1:
resolved "https://registry.yarnpkg.com/jsesc/-/jsesc-2.5.2.tgz#80564d2e483dacf6e8ef209650a67df3f0c283a4"
integrity sha512-OYu7XEzjkCQ3C5Ps3QIZsQfNpqoJyZZA99wd9aWd05NCtC5pWOkShK2mkL6HXQR6/Cy2lbNdPlZBpuQHXE63gA==

jsforce@^2.0.0-beta.27:
version "2.0.0-beta.27"
resolved "https://registry.yarnpkg.com/jsforce/-/jsforce-2.0.0-beta.27.tgz#ba822b18b77bea4529491060a9b07bc1009735ac"
integrity sha512-d9dDWWeHwayRKPo8FJBAIUyk8sNXGSHwdUjR6al3yK0YKci27Jc1XfNaQTxEAuymHQJVaCb1gxTKqmA4uznFdQ==
jsforce@^2.0.0-beta.28:
version "2.0.0-beta.28"
resolved "https://registry.yarnpkg.com/jsforce/-/jsforce-2.0.0-beta.28.tgz#5fd8d9b8e5efc798698793b147e00371f3d74e8f"
integrity sha512-tTmKRhr4yWNinhmurY/tiiltLFQq9RQ+gpYAt3wjFdCGjzd49/wqYQIFw4SsI3+iLjxXnc0uTgGwdAkDjxDWnA==
dependencies:
"@babel/runtime" "^7.12.5"
"@babel/runtime-corejs3" "^7.12.5"
Expand Down

3 comments on commit 8e346ca

@svc-cli-bot
Copy link
Contributor

Choose a reason for hiding this comment

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

Logger Benchmarks - ubuntu-latest

Benchmark suite Current: 8e346ca Previous: e7e23bb Ratio
Child logger creation 530040 ops/sec (±0.21%) 397406 ops/sec (±18.39%) 0.75
Logging a string on root logger 493295 ops/sec (±10.24%) 414236 ops/sec (±12.52%) 0.84
Logging an object on root logger 351593 ops/sec (±20.62%) 294898 ops/sec (±13.67%) 0.84
Logging an object with a message on root logger 212392 ops/sec (±13.57%) 163921 ops/sec (±18.79%) 0.77
Logging an object with a redacted prop on root logger 13921 ops/sec (±186.34%) 164027 ops/sec (±17.26%) 11.78
Logging a nested 3-level object on root logger 210216 ops/sec (±13.97%) 117715 ops/sec (±23.84%) 0.56

This comment was automatically generated by workflow using github-action-benchmark.

@svc-cli-bot
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Logger Benchmarks - ubuntu-latest'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 8e346ca Previous: e7e23bb Ratio
Logging an object with a redacted prop on root logger 13921 ops/sec (±186.34%) 164027 ops/sec (±17.26%) 11.78

This comment was automatically generated by workflow using github-action-benchmark.

@svc-cli-bot
Copy link
Contributor

Choose a reason for hiding this comment

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

Logger Benchmarks - windows-latest

Benchmark suite Current: 8e346ca Previous: e7e23bb Ratio
Child logger creation 354069 ops/sec (±11.38%) 415626 ops/sec (±1.26%) 1.17
Logging a string on root logger 417103 ops/sec (±9.66%) 452588 ops/sec (±16.61%) 1.09
Logging an object on root logger 305300 ops/sec (±18.95%) 358573 ops/sec (±16.15%) 1.17
Logging an object with a message on root logger 212172 ops/sec (±13.30%) 293219 ops/sec (±8.92%) 1.38
Logging an object with a redacted prop on root logger 233725 ops/sec (±21.69%) 291480 ops/sec (±18.57%) 1.25
Logging a nested 3-level object on root logger 187343 ops/sec (±21.04%) 2803 ops/sec (±206.79%) 0.014961861398611104

This comment was automatically generated by workflow using github-action-benchmark.

Please sign in to comment.