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

Fix yarn workspaces #517

Merged
merged 8 commits into from
Aug 29, 2024
Merged

Fix yarn workspaces #517

merged 8 commits into from
Aug 29, 2024

Conversation

filip131311
Copy link
Collaborator

@filip131311 filip131311 commented Aug 27, 2024

This PR fixes node_modules detection for yarn workspaces and adds a working example of a set up in which workspaces are used.

relates to: #514

Copy link

vercel bot commented Aug 27, 2024

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 Aug 29, 2024 10:57am

const packageManager = require(path.join(workspace, "package.json")).packageManager;

if (packageManager) {
const regex = /^([a-zA-Z]+)@/;
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to have an example in the comment above, like "yarn@3.6.4". I initially thought this was related to @xxx/yyy format.

regex name doesn't help at all, we can basically inline it IMO.

Comment on lines 46 to 49
pathExists(resolve(workspace, "yarn.lock")),
pathExists(resolve(workspace, "package-lock.json")),
pathExists(resolve(workspace, "pnpm-lock.yaml")),
pathExists(resolve(workspace, "bun.lockb")),
Copy link
Member

Choose a reason for hiding this comment

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

Tiny thing to consider: we could read the dir and the check existence in JS, should be faster than doing 4 syscalls. Probably doesn't matter though.

import fs from "node:fs/promises";

async f() {
  const lockFiles = new Map(
    Object.entries({
      "yarn.lock": "yarn",
      "package-lock.json": "npm",
      "pnpm-lock.yaml": "pnpm",
      "bun.lockb": "bun",
    } as const)
  );

  const files = await fs.readdir(workspace);
  for (const file of files) {
    const packageManager = lockFiles.get(file);
    if (packageManager) {
      return packageManager;
    }
  }
}

Comment on lines 28 to 31
const packageJsonPath = path.join(currentDir, "package.json");
if (!existsSync(packageJsonPath)) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

I think we should just read the file and catch if doesn't exist, something like that:

function walkDirectoriesUntilRoot(
  start: string,
  shouldStop: (current: string) => boolean
): string | undefined {
  let currentDir = start;
  let parentDir = path.resolve(currentDir, "..");
  while (parentDir !== currentDir) {
    currentDir = parentDir;
    parentDir = path.resolve(currentDir, "..");

    if (shouldStop(currentDir)) {
      return currentDir;
    }
  }
  return undefined;
}

function findWorkspace(appRoot: string) {
  return walkDirectoriesUntilRoot(appRoot, (currentDir) => {
    try {
      const packageJsonPath = path.join(currentDir, "package.json");
      const hasWorkspaces = require(packageJsonPath).workspaces !== undefined;
      return hasWorkspaces;
    } catch (error) {
      // No package.json
      return false;
    }
  });
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i did that just reaarengedsome functions to reuse code

const openLaunchConfigButton = "Open Launch Configuration";
window
.showErrorMessage(
`multiple reat-native applications were detected in the workspace, ${appRootCandidates[0]} were chosen, but you can change the root of your application in lunch configuration.`,
Copy link
Member

Choose a reason for hiding this comment

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

add some text that says that in order to get rid of this error, people should specify app root in the launch configuration

Copy link
Member

Choose a reason for hiding this comment

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

Also, "lunch configuration" typo. 🥪

@filip131311 filip131311 merged commit 75a6fe9 into main Aug 29, 2024
3 checks passed
@filip131311 filip131311 deleted the @Filip131311/FixYarnWorkspaces branch August 29, 2024 10:58
@DorianMazur
Copy link

This PR probably broke react-native-ide. Now node_modules don't work. I had to downgrade to 0.17

@filip131311
Copy link
Collaborator Author

@DorianMazur hello, thank you so much for your feedback would you mind sharing React Native IDE logs from 0.18 version with us? you can find them in the output section of your vscode. Why do you think it is this PR that broke it for you?

@DorianMazur
Copy link

DorianMazur commented Sep 12, 2024

I'm using yarn with corepack in a project and it basically can't find node_modules. After downgrading to 0.17 everything works like a charm.

I analyzed all commits between versions 0.18 and 0.17 and found only one that refers to node_modules. This one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants