Skip to content

Commit

Permalink
Remove stalePods checks from dependency manager to fix rebuild issues (
Browse files Browse the repository at this point in the history
…#684)

In #457, we added a concept of stale pods flag, that was used to mark
whether pod install command should run as a consequence of node_modules.

Since that time, I got bitten by this setting causing unexpected
rebuilds to the point where some projects I'm testing run native build
every single time despite no change in fingerprint.

In general I don't like the fact we are adding additional state to
facilitate checks that seemingly can be done offline. From my testing,
I'm now a bit confused as to what was the purpose of this flag as #457
lacks a proper description and we also don't have any inline comments
about this flag and related code. Therefore I'm assuming that the
purpose of it was that we trigger pod installation if we had to
reinstall node_modules. However, this still seem to not be necessary, as
fingerprint seems to deal with this type of scenarios well.
Specifically, when we add new native dependencies that get installed in
node installation phase, fingerprint will detect that and trigger native
rebuild.

On top of that, this mechanism, regardless of whether I guessed the
original reason for it, is there to prevent from stale builds being
used. However, it still appears like it is much easier for the users to
recover from issues that may arise from stale native builds than to
recover from persistent rebuilds taking many minutes. In the former
scenario, all you need to do is to trigger "clean build" reload. In the
latter, there's no way for the user to debug that, and I experienced
that myself.

Fixes # (issue)
1. Test expo-router app, add native dependency, test again
2. I also tested this on reanimated repo example apps with yarn and
that's where I've been experiencing the described issue of consistent
rebuilds.
  • Loading branch information
kmagiera authored Nov 4, 2024
1 parent 7e30142 commit d7e1c0d
Showing 1 changed file with 1 addition and 18 deletions.
19 changes: 1 addition & 18 deletions packages/vscode-extension/src/dependency/DependencyManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import semver, { SemVer } from "semver";
import { Logger } from "../Logger";
import { EMULATOR_BINARY } from "../devices/AndroidEmulatorDevice";
import { command, lineReader } from "../utilities/subprocess";
import { extensionContext, getAppRootFolder } from "../utilities/extensionContext";
import { getAppRootFolder } from "../utilities/extensionContext";
import { getIosSourceDir } from "../builders/buildIOS";
import { isExpoGoProject } from "../builders/expoGo";
import {
Expand All @@ -27,11 +27,8 @@ import { CancelToken } from "../builders/cancelToken";
import { getAndroidSourceDir } from "../builders/buildAndroid";
import { Platform } from "../utilities/platform";

const STALE_PODS = "stalePods";

export class DependencyManager implements Disposable, DependencyManagerInterface {
// React Native prepares build scripts based on node_modules, we need to reinstall pods if they change
private stalePods = extensionContext.workspaceState.get<boolean>(STALE_PODS) ?? false;
private eventEmitter = new EventEmitter();
private packageManagerInternal: PackageManagerInfo | undefined;

Expand Down Expand Up @@ -140,8 +137,6 @@ export class DependencyManager implements Disposable, DependencyManagerInterface
return false;
}

await this.setStalePodsAsync(true);

this.emitEvent("nodeModules", { status: "installing", isOptional: false });

// all managers support the `install` command
Expand Down Expand Up @@ -183,17 +178,10 @@ export class DependencyManager implements Disposable, DependencyManagerInterface
return;
}

await this.setStalePodsAsync(false);

this.emitEvent("pods", { status: "installed", isOptional: false });
Logger.debug("Project pods installed");
}

private async setStalePodsAsync(stale: boolean) {
this.stalePods = stale;
await extensionContext.workspaceState.update(STALE_PODS, stale);
}

private async getPackageManager() {
if (!this.packageManagerInternal) {
this.packageManagerInternal = await resolvePackageManager();
Expand Down Expand Up @@ -273,11 +261,6 @@ export class DependencyManager implements Disposable, DependencyManagerInterface
return true;
}

if (requiresNativeBuild && this.stalePods) {
this.emitEvent("pods", { status: "notInstalled", isOptional: false });
return false;
}

const appRootFolder = getAppRootFolder();
const iosDirPath = getIosSourceDir(appRootFolder);

Expand Down

0 comments on commit d7e1c0d

Please sign in to comment.