Skip to content

Commit 2000f91

Browse files
committed
refactor(package-managers): select the package manager and resolve packages synchronously
Nothing about locating an installed package is asynchronous; the only async link was the dispatcher reading the "packageManager" user setting through the settings lock. JsonFileSettingsService gains a lock-free getSettingValueSync for settings that only change through explicit user commands, and the dispatcher now picks its implementation lazily and synchronously, dropping the @cache/@invokeInit init dance. getInstalledPackagePath is therefore synchronous on the contract, which unwinds the async that had been threaded through doctor, plugins-service, the bundler, the test runners, preview and android-plugin-build-service, and lets the last two direct users of the resolution helper move onto the contract: getRuntimePackage in project-data-service (resolved lazily via the injector, as the service is constructed everywhere) and the transitive walk in node-modules-dependencies-builder.
1 parent 14a41fa commit 2000f91

41 files changed

Lines changed: 218 additions & 192 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

PublicAPI.md

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -579,16 +579,15 @@ Locates a package the way the selected package manager laid it out on disk, so c
579579
/**
580580
* @param {string} packageName The name of the package.
581581
* @param {string} fromDir The directory whose dependencies are searched, usually the project directory.
582-
* @return {Promise<string>} The absolute path of the package directory, or null when it is not installed.
582+
* @return {string} The absolute path of the package directory, or null when it is not installed.
583583
*/
584-
getInstalledPackagePath(packageName: string, fromDir: string): Promise<string>;
584+
getInstalledPackagePath(packageName: string, fromDir: string): string;
585585
```
586586
587587
* Usage:
588588
```JavaScript
589-
tns.packageManager.getInstalledPackagePath("@nativescript/core", "/tmp/myProject").then(pathToPackage => {
590-
console.log(pathToPackage ? `Installed at ${pathToPackage}` : "Not installed");
591-
});
589+
const pathToPackage = tns.packageManager.getInstalledPackagePath("@nativescript/core", "/tmp/myProject");
590+
console.log(pathToPackage ? `Installed at ${pathToPackage}` : "Not installed");
592591
```
593592
594593
### view

lib/commands/preview.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ export class PreviewCommand extends Command({
3737
await this.installLatestPreviewCLI();
3838
}
3939

40-
const previewCLIPath = await this.getPreviewCLIPath();
40+
const previewCLIPath = this.getPreviewCLIPath();
4141

4242
if (!previewCLIPath) {
4343
await this.failMissingPreviewCLI();
@@ -58,7 +58,7 @@ export class PreviewCommand extends Command({
5858
);
5959
}
6060

61-
private getPreviewCLIPath(): Promise<string> {
61+
private getPreviewCLIPath(): string {
6262
return this.$packageManager.getInstalledPackagePath(
6363
PREVIEW_CLI_PACKAGE,
6464
this.$projectData.projectDir,

lib/commands/test-init.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ export class TestInitCommand extends Command({
137137
path: this.$options.path,
138138
});
139139

140-
const modulePath = await this.$packageManager.getInstalledPackagePath(
140+
const modulePath = this.$packageManager.getInstalledPackagePath(
141141
mod.name,
142142
projectDir,
143143
);

lib/commands/test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ async function canExecuteTestCommand(
106106

107107
if ($vitestExecutionService.isVitestProject($projectData)) {
108108
const canStartTestRun =
109-
await $vitestExecutionService.canStartTestRun($projectData);
109+
$vitestExecutionService.canStartTestRun($projectData);
110110
if (!canStartTestRun) {
111111
$errors.fail({
112112
formatStr:

lib/common/definitions/json-file-settings-service.d.ts

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,11 @@ interface IJsonFileSettingsService {
1313
settingName: string,
1414
cacheOpts?: ICacheTimeoutOpts
1515
): Promise<T>;
16+
/**
17+
* Reads a setting without taking the settings lock. Suitable for values that
18+
* only change through explicit user commands, where a torn read is harmless.
19+
*/
20+
getSettingValueSync<T>(settingName: string): T;
1621
saveSetting<T>(
1722
key: string,
1823
value: T,

lib/common/services/json-file-settings-service.ts

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,28 @@ export class JsonFileSettingsService implements IJsonFileSettingsService {
5656
);
5757
}
5858

59+
public getSettingValueSync<T>(settingName: string): T {
60+
if (!this.jsonSettingsData && this.$fs.exists(this.jsonSettingsFilePath)) {
61+
try {
62+
this.jsonSettingsData = parseJson(
63+
this.$fs.readText(this.jsonSettingsFilePath)
64+
);
65+
} catch (err) {
66+
this.$logger.trace(
67+
`Error while trying to parse ${this.jsonSettingsFilePath}. Err is: ${err}`
68+
);
69+
return null;
70+
}
71+
}
72+
73+
if (this.jsonSettingsData && _.has(this.jsonSettingsData, settingName)) {
74+
const data = this.jsonSettingsData[settingName];
75+
return data.modifiedByCacheMechanism ? data.value : data;
76+
}
77+
78+
return null;
79+
}
80+
5981
public async saveSetting<T>(
6082
key: string,
6183
value: T,

lib/common/test/unit-tests/services/json-file-settings-service.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,37 @@ describe("jsonFileSettingsService", () => {
6565
Date.now = originalDateNow;
6666
});
6767

68+
describe("getSettingValueSync", () => {
69+
it("returns the stored value without going through the lock", () => {
70+
const testInjector = createTestInjector();
71+
dataInFile[jsonFileSettingsPath] = { prop1: "value1" };
72+
const lockService = testInjector.resolve("lockService");
73+
lockService.executeActionWithLock = () => {
74+
throw new Error("lock must not be used for sync reads");
75+
};
76+
77+
const jsonFileSettingsService =
78+
testInjector.resolve<IJsonFileSettingsService>(
79+
"jsonFileSettingsService",
80+
{ jsonFileSettingsPath }
81+
);
82+
assert.equal(jsonFileSettingsService.getSettingValueSync("prop1"), "value1");
83+
assert.isNull(jsonFileSettingsService.getSettingValueSync("missing"));
84+
});
85+
86+
it("returns null when the settings file does not exist", () => {
87+
const testInjector = createTestInjector();
88+
const fs = testInjector.resolve("fs");
89+
fs.exists = () => false;
90+
const jsonFileSettingsService =
91+
testInjector.resolve<IJsonFileSettingsService>(
92+
"jsonFileSettingsService",
93+
{ jsonFileSettingsPath }
94+
);
95+
assert.isNull(jsonFileSettingsService.getSettingValueSync("prop1"));
96+
});
97+
});
98+
6899
describe("getSettingValue", () => {
69100
it("returns correct data without cache", async () => {
70101
dataInFile = { [jsonFileSettingsPath]: { prop1: 1 } };

lib/contracts/doctor-service.ts

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,5 @@ export abstract class DoctorService {
3434
}): Promise<boolean>;
3535

3636
/** Checks and notifies users of deprecated short imports in their app. */
37-
abstract checkForDeprecatedShortImportsInAppDir(
38-
projectDir: string,
39-
): Promise<void>;
37+
abstract checkForDeprecatedShortImportsInAppDir(projectDir: string): void;
4038
}

lib/contracts/package-manager.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -103,12 +103,12 @@ export abstract class PackageManager {
103103
* Locates a package the way the package manager laid it out on disk.
104104
* @param {string} packageName The name of the package.
105105
* @param {string} fromDir The directory whose dependencies are searched, usually the project directory.
106-
* @return {Promise<string>} The absolute path of the package directory, or null when it is not installed.
106+
* @return {string} The absolute path of the package directory, or null when it is not installed.
107107
*/
108108
abstract getInstalledPackagePath(
109109
packageName: string,
110110
fromDir: string,
111-
): Promise<string>;
111+
): string;
112112

113113
/**
114114
* Gets the name of the package manager used for the current process.

lib/controllers/prepare-controller.ts

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -494,11 +494,10 @@ export class PrepareController
494494
SCOPED_ANDROID_RUNTIME_NAME;
495495
}
496496
// try reading from installed runtime first before reading from the npm registry...
497-
const installedRuntimePath =
498-
await this.$packageManager.getInstalledPackagePath(
499-
runtimePackageName,
500-
projectData.projectDir,
501-
);
497+
const installedRuntimePath = this.$packageManager.getInstalledPackagePath(
498+
runtimePackageName,
499+
projectData.projectDir,
500+
);
502501

503502
if (installedRuntimePath) {
504503
installedRuntimePackageJSON = this.$fs.readJson(

0 commit comments

Comments
 (0)