Skip to content

Commit d2def6b

Browse files
rigor789edusperoni
authored andcommitted
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 aa7e55c commit d2def6b

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
@@ -38,7 +38,7 @@ export class PreviewCommand extends Command({
3838
await this.installLatestPreviewCLI();
3939
}
4040

41-
const previewCLIPath = await this.getPreviewCLIPath();
41+
const previewCLIPath = this.getPreviewCLIPath();
4242

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

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

lib/commands/test-init.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ export class TestInitCommand extends Command({
134134
path: this.options.path,
135135
});
136136

137-
const modulePath = await this.$packageManager.getInstalledPackagePath(
137+
const modulePath = this.$packageManager.getInstalledPackagePath(
138138
mod.name,
139139
projectDir,
140140
);

lib/commands/test.ts

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

109109
if ($vitestExecutionService.isVitestProject($projectData)) {
110110
const canStartTestRun =
111-
await $vitestExecutionService.canStartTestRun($projectData);
111+
$vitestExecutionService.canStartTestRun($projectData);
112112
if (!canStartTestRun) {
113113
$errors.fail({
114114
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
@@ -497,11 +497,10 @@ export class PrepareController
497497
SCOPED_ANDROID_RUNTIME_NAME;
498498
}
499499
// try reading from installed runtime first before reading from the npm registry...
500-
const installedRuntimePath =
501-
await this.$packageManager.getInstalledPackagePath(
502-
runtimePackageName,
503-
projectData.projectDir,
504-
);
500+
const installedRuntimePath = this.$packageManager.getInstalledPackagePath(
501+
runtimePackageName,
502+
projectData.projectDir,
503+
);
505504

506505
if (installedRuntimePath) {
507506
installedRuntimePackageJSON = this.$fs.readJson(

0 commit comments

Comments
 (0)