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) pnp workspace support #1393

Merged
merged 2 commits into from
Jan 24, 2023
Merged

fix(yarn) pnp workspace support #1393

merged 2 commits into from
Jan 24, 2023

Conversation

mufasa71
Copy link
Contributor

@mufasa71 mufasa71 commented Dec 3, 2022

Yarn berry works great with single mode, but does not works if inside workspace directory. I found that this line does not respect root monorepo:

pub fn is_yarn_pnp(&self) -> bool {

as it expect .pnp.js on same level as root dir. But workspace dir does not have .pnp.js hence it works fine with yarn berry. like yarn why packagename

Copy link
Contributor

@charlespierce charlespierce left a comment

Choose a reason for hiding this comment

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

Thanks for taking this on! A suggestion to avoid duplicating work. Also, there may be some merge conflicts with #1388 which is adding another file to the checks here, so if that is merged first, it will need to be resolved.

base_dir.join(".pnp.js").exists()
|| base_dir.join(".pnp.cjs").exists()
|| self
.workspace_roots()
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this winds up duplicating work: self.workspace_roots() includes as the first entry the parent of self.manifest_file, so we're re-checking that. Instead, I think this check that you have is already exactly what we want (and can replace the whole self.manifest_file... check)

self
    .workspace_roots()
    .any(|dir| dir.join(".pnp.cjs").exists() || dir.join(".pnp.js").exists())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks I applied changes, will wait until #1388 is merged to resolve conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like #1388 is merged, can we merge this now?

@charlespierce charlespierce merged commit dc6f5b5 into volta-cli:main Jan 24, 2023
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.

None yet

2 participants