Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: handle multiple previews at the same time #421

Merged
merged 4 commits into from
Feb 13, 2025

Conversation

gonpombo8
Copy link
Contributor

@gonpombo8 gonpombo8 commented Feb 4, 2025

also error handling when importing scene and its not accesible

Copy link

github-actions bot commented Feb 4, 2025

Test this pull request on windows-latest

Download the correct version for your architecture:

win-x64

Copy link

github-actions bot commented Feb 4, 2025

Test this pull request on macos-latest

Download the correct version for your architecture:

mac-arm64
mac-x64

Click here if you don't know which version to download

For running this unsigned version of the app, you will need to run the xattr command on it:

  1. Extract the app from the downloaded .dmg file (double-click it)
  2. Place the extracted app anywhere you like in your file system
  3. Open a terminal on the directory where the app is
  4. Run xattr -c app-name, replacing "app-name" for the actual name of the app
  5. Double-click the app ✅

}
} catch (_) {
missing.push(_path);
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it going to enter here when the fs.readdir throws an error?

In that case, should we skip the path because it isn't accessible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i was following the same approach as the code does when the path doesn't exist or its unaccessible.

      if (!(await exists(_path))) {
        // _path doesn't exist
        missing.push(_path);

@gonpombo8 gonpombo8 force-pushed the fix/error-hanlding-import-scene branch from cb24199 to 606bddc Compare February 12, 2025 15:54
@@ -22,24 +22,41 @@ export async function init(path: string, repo?: string) {
await initCommand.wait();
}

export let previewServer: Child | null = null;
export async function killPreview(path: string) {
Copy link
Contributor

@nicoecheza nicoecheza Feb 12, 2025

Choose a reason for hiding this comment

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

what do you think of something like this? just replaced the cache with a Map and added some types

interface PreviewData {
  child: Child;
  previewURL: string;
}

export const previewCache = new Map<string, PreviewData>();

export async function killPreview(path: string) {
  const preview = previewCache.get(path);
  if (preview?.child) {
    await preview.child.kill().catch(() => {});
  }
  previewCache.delete(path);
}

export async function killAllPreviews(): Promise<void> {
  const killPromises: Promise<void>[] = [];
  for (const preview of previewCache.values()) {
    killPromises.push(preview.child.kill().catch(() => {}));
  }

  await Promise.all(killPromises);
  previewCache.clear();
}

async function createPreviewProcess(path: string): Promise<PreviewData> {
  const child = run('@dcl/sdk-commands', 'sdk-commands', {
    args: ['start', '--explorer-alpha', '--hub'],
    cwd: path,
    workspace: path,
    env: await getEnv(path),
  });

  const dclLauncherURL = /decentraland:\/\/([^\s\n]*)/i;
  const resultLogs = await child.waitFor(dclLauncherURL, /CliError/i);
  const urlMatch = resultLogs.match(dclLauncherURL);

  return {
    child,
    previewURL: urlMatch?.[1] ?? '',
  };
}

export async function start(path: string, retry = true) {
  try {
    const existingPreview = previewCache.get(path);
    if (existingPreview?.child.alive() && existingPreview.previewURL) {
      await dclDeepLink(existingPreview.previewURL);
      return;
    }

    await killPreview(path);

    const preview = await createPreviewProcess(path);
    previewCache.set(path, preview);

  } catch (error) {
    previewCache.delete(path);
    if (retry) {
      log.info('[CLI] Something went wrong trying to start preview:', (error as Error).message);
      await install(path);
      await start(path, false);
    } else {
      throw error;
    }
  }
}

and replace killPreview with killAllPreview on packages/main/src/index.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whats the diff between a map and a record ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

moving everything to diff functions that only are used 1 time feels unnecessary imo

@@ -405,6 +405,14 @@ async function handleData(buffer: Buffer, matchers: Matcher[], type: StreamType)
}
}

export async function dclDeepLink(deepLink: string) {
try {
await exec(`open decentraland://"${deepLink}"`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

in Windows we have to use start "decentraland://${deepLink}", does the exec handle it automatically for each OS?

Copy link
Contributor Author

@gonpombo8 gonpombo8 Feb 12, 2025

Choose a reason for hiding this comment

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

YES !!!!!!!!!!!!!!! that was a TODO that totally forgot about! thanks gabi

@gonpombo8 gonpombo8 changed the title fix: error handling when importing scene feat: handle multiple previews at the same time Feb 13, 2025
@nicoecheza nicoecheza merged commit c842930 into main Feb 13, 2025
10 checks passed
@nicoecheza nicoecheza deleted the fix/error-hanlding-import-scene branch February 13, 2025 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants