Improve a flow of loading devices and xcode detection #563

filip131311 · 2024-09-19T15:13:16Z

this PR addresses a few issues with the loading devices flow:

checkIfCLIInstalled no longer logs errors when returning false instead just logs with debug level, this change makes the logs more alined with expected results for setups without xcode
initial device load is performed with force, as xcode installation could change between session of the ide
initial loading screen no longer "jumps" between states
removed DYLD_FRAMEWORK_PATH as it is no longer needed
trySelectingInitialDevice() creates "devicesChanged" listeners before selectingInitial device is complete, so it catches cases in which lastDeviceId points to the device that no longer exists (e.g. because of xcode update)

vercel · 2024-09-19T15:13:23Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
react-native-ide	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 24, 2024 9:47am

jakub-gonet · 2024-09-20T09:44:28Z

packages/vscode-extension/src/dependency/DependencyManager.ts

@@ -428,9 +425,12 @@ export class DependencyManager implements Disposable {
 export async function checkIfCLIInstalled(cmd: string, options: Record<string, unknown> = {}) {
  try {
    // We are not checking the stderr here, because some of the CLIs put the warnings there.
-    const { stdout } = await command(cmd, { encoding: "utf8", ...options });
-    return !!stdout.length;
+    const { stdout } = await command(cmd, { encoding: "utf8", quiet: true, ...options });


quiet name needs to change eventually to something like printStdErr, I'd assume if quiet is true we only get status code on exit.

jakub-gonet · 2024-09-20T09:45:12Z

packages/vscode-extension/src/dependency/DependencyManager.ts

-    const { stdout } = await command(cmd, { encoding: "utf8", ...options });
-    return !!stdout.length;
+    const { stdout } = await command(cmd, { encoding: "utf8", quiet: true, ...options });
+    const result = !!stdout.length;


Please be more explicit:

const hasOutput = stdout.lenght > 0;

jakub-gonet · 2024-09-20T09:45:42Z

packages/vscode-extension/src/dependency/DependencyManager.ts

-    return !!stdout.length;
+    const { stdout } = await command(cmd, { encoding: "utf8", quiet: true, ...options });
+    const result = !!stdout.length;
+    Logger.debug(`CLI: ${cmd} ${result ? "" : "not"} installed `);


Nit: "CLI: " prefix is unnecessary.

I think it is useful to have prefixes that allow to serach what part of our code generated the log. We have. a growing code base and most of that log is generated dynamically so searching for it would be hard without it.

jakub-gonet · 2024-09-20T09:49:53Z

packages/vscode-extension/src/devices/DeviceManager.ts

@@ -125,11 +128,11 @@ export class DeviceManager implements DeviceManagerInterface {
    return getAvailableIosRuntimes();
  }

-  public async listAllDevices() {
+  public async listAllDevices(force?: boolean) {


Maybe forceReload?

jakub-gonet · 2024-09-20T09:52:10Z

packages/vscode-extension/src/webview/views/PreviewView.tsx

-          {devicesNotFound ? <DevicesNotFoundView /> : <VSCodeProgressRing />}
-        </div>
+        <LoadingComponent
+          finishedInitialLoad={finishedInitialLoad}


Why we're checking for finishedInitialLoad twice in the ternary and in the component?

there is more then one reason for rendering the loading component, but not all of them behave the same.

packages/vscode-extension/src/dependency/DependencyManager.ts

kmagiera · 2024-09-20T12:14:27Z

packages/vscode-extension/src/devices/DeviceManager.ts

-          })
-        : Promise.resolve([]);
+
+    const shouldLoadSimulators = Platform.OS === "macos" && (await checkXcodeExists());


Maybe we can add additional log here when checkXcodeExists fails. For example "Couldn't list iOS simulators as XCode installation wasn't found" – this will allow us to track issues in case those checks aren't accurate

kmagiera · 2024-09-20T12:15:38Z

packages/vscode-extension/src/webview/providers/ProjectProvider.tsx

@@ -26,6 +26,7 @@ const ProjectContext = createContext<ProjectContextProps>({
      longitude: 19.965474,
      isDisabled: false,
    },
+    locale: "en_US",


why is this change here?

It was missed before and ts was not happy, it does not effect the webview much at all as it never uses default context.

kmagiera · 2024-09-20T12:19:55Z

packages/vscode-extension/src/devices/DeviceManager.ts

    const devices = extensionContext.globalState.get(DEVICE_LIST_CACHE_KEY) as
      | DeviceInfo[]
      | undefined;
-    if (devices) {
+    if (devices && !forceReload) {


this isn't accurate as we always load the devices. The difference is that we return the updated list or let it update asynchronously. I'm a bit hesitant about this change as from what I remember, we added device caching as it was slowing down the initial startup significantly. As it isn't documented in PR description I wonder why you're changing this behavior?

When user had an iPhone selected when they last used IDE, but something happend to xcode (e.g. user started using beta but not updated the xcode-select) application would select an iPhone anyway as it is the most recent device and would get stuck on loading screen.

What dou you think about forceLoad instead of forceReload

I think we should optimize for the most common case in which people don't change their xcode location. If there are cases when it happens, I understand they will get an error when trying to load the simulator, and by that time we will have the list refetched. If that's not the case we should adjust the logic such that the worst thing that can happen if you mess up the simjulator cache or xcode is that you get an error loading the simulator instead of slowing down all the time to interact for everyone else.

kmagiera

Looks mostly good, left a few comments inline

kmagiera · 2024-09-23T10:23:27Z

packages/vscode-extension/src/devices/DeviceManager.ts

+
+    let shouldLoadSimulators = Platform.OS === "macos";
+
+    if (!(await checkXcodeExists())) {


do we even want to run those checks if we're not on a Mac?

Perhaps doing it this way would be cleaner:

const shouldLoadSimulators = Platform.OS === "macos" && (await checkXcodeExists());

Then you can move the log to be printed in checkXcodeExists

I did this in this way to address your previous comment about adding logging in !checkXcodeExist() scennario

kmagiera · 2024-09-23T10:32:52Z

packages/vscode-extension/src/project/project.ts

+    const selectInitialDevicePromise = selectInitialDevice(devices);
+
+    const listener = async (newDevices: DeviceInfo[]) => {
+      if (await selectInitialDevicePromise) {
+        this.deviceManager.removeListener("devicesChanged", listener);
+        return;
+      }
+      if (await selectInitialDevice(newDevices)) {
+        this.deviceManager.removeListener("devicesChanged", listener);
+      }
+    };
+    this.deviceManager.addListener("devicesChanged", listener);
+
+    if (await selectInitialDevicePromise) {
+      this.deviceManager.removeListener("devicesChanged", listener);


This code doesn't do the same thing as the one on the left. In general I'm most afraid of the changes in awaiting that you're adding here. Not saying we shouldn't have them, but they require some explanation in the PR description, such that we can understand why we're changing the await flow.

This part is the most complex change and it introduces some unusual pattern (that we, albeit, use in other places but typically with some explanation) where we create promise and then await inside of an arrow function.

I think that given this method already awaits selectDevice (which wasn't the case before this change) , we can just add the listener before calling selectInitialDevice ?

kmagiera

lgtm 👍

This PR fixes a mistake introduced in #563, the return value of `selectDevice` should indicate if device was chosen successfully, not if processes on it are successful otherwise if build fails and the selected device was set by `trySelectingInitiallDevice` we are getting stuck in infinite loop.

init

beccd30

filip131311 changed the title ~~Improve a flow of loading devices~~ Improve a flow of loading devices and xcode detection Sep 19, 2024

rmove unnecessery env variable

e478f6f

vercel bot deployed to Preview September 19, 2024 15:19 View deployment

docs

d18d9f2

vercel bot deployed to Preview September 19, 2024 15:24 View deployment

filip131311 requested review from kmagiera and jakub-gonet and removed request for kmagiera September 20, 2024 08:57

jakub-gonet reviewed Sep 20, 2024

View reviewed changes

filip131311 commented Sep 20, 2024

View reviewed changes

packages/vscode-extension/src/dependency/DependencyManager.ts Outdated Show resolved Hide resolved

Update packages/vscode-extension/src/dependency/DependencyManager.ts

cccd83f

vercel bot deployed to Preview September 20, 2024 10:05 View deployment

changes after CR

33bdf03

vercel bot deployed to Preview September 20, 2024 11:44 View deployment

kmagiera reviewed Sep 20, 2024

View reviewed changes

changes after CR

e86461f

vercel bot deployed to Preview September 20, 2024 13:39 View deployment

changes after CR

d284655

vercel bot deployed to Preview September 23, 2024 09:00 View deployment

filip131311 requested a review from kmagiera September 23, 2024 10:12

kmagiera reviewed Sep 23, 2024

View reviewed changes

add coments

c50b2f6

vercel bot deployed to Preview September 23, 2024 11:55 View deployment

filip131311 added 2 commits September 23, 2024 15:16

add coments

2326812

add coments

7294163

vercel bot deployed to Preview September 23, 2024 13:17 View deployment

filip131311 requested a review from kmagiera September 23, 2024 19:05

changes after review

ffb3967

vercel bot deployed to Preview September 24, 2024 09:39 View deployment

replace json.stringify with lodash.isEqual

4599713

vercel bot deployed to Preview September 24, 2024 09:47 View deployment

kmagiera approved these changes Sep 24, 2024

View reviewed changes

filip131311 merged commit 028091e into main Sep 24, 2024
3 checks passed

filip131311 deleted the @Filip131311/ImproveAFlowOfLoadingDevices branch September 24, 2024 09:55

filip131311 mentioned this pull request Sep 24, 2024

Fix infinite reload #568

Merged

filip131311 mentioned this pull request Oct 10, 2024

Stuck on spinner when IDE failed to start the first time #174

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve a flow of loading devices and xcode detection #563

Improve a flow of loading devices and xcode detection #563

filip131311 commented Sep 19, 2024 •

edited

Loading

vercel bot commented Sep 19, 2024 •

edited

Loading

jakub-gonet Sep 20, 2024

jakub-gonet Sep 20, 2024

jakub-gonet Sep 20, 2024

filip131311 Sep 20, 2024

jakub-gonet Sep 20, 2024

jakub-gonet Sep 20, 2024

filip131311 Sep 20, 2024

kmagiera Sep 20, 2024

kmagiera Sep 20, 2024

filip131311 Sep 20, 2024

kmagiera Sep 20, 2024

filip131311 Sep 20, 2024

kmagiera Sep 20, 2024

kmagiera left a comment

kmagiera Sep 23, 2024

filip131311 Sep 23, 2024

kmagiera Sep 23, 2024

kmagiera left a comment


		let shouldLoadSimulators = Platform.OS === "macos";

		if (!(await checkXcodeExists())) {

Improve a flow of loading devices and xcode detection #563

Improve a flow of loading devices and xcode detection #563

Conversation

filip131311 commented Sep 19, 2024 • edited Loading

vercel bot commented Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmagiera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmagiera left a comment

Choose a reason for hiding this comment

filip131311 commented Sep 19, 2024 •

edited

Loading

vercel bot commented Sep 19, 2024 •

edited

Loading