Skip to content

Conversation

@varungandhi-src
Copy link
Contributor

@varungandhi-src varungandhi-src commented Oct 12, 2022

Fixes #180.

Test plan

Manually tested against the Sourcegraph monorepo with Yarn 3 which seems to have been made the default around Sept 1 (http://github.com/sourcegraph/sourcegraph/pull/39728). The last scip-typescript upload for sg/sg was on Sept 2 (https://sourcegraph.sourcegraph.com/github.com/sourcegraph/sourcegraph/-/code-graph/uploads?query=scip-typescript), so it seems like this job has been broken since then.

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

What's the relationship with this PR here #168? We already support Yarn v3 with --yarn-berry-workspaces

@varungandhi-src
Copy link
Contributor Author

Hmm, I didn't know what Yarn Berry was. I guess it is a little weird to have two flags instead of one...

@varungandhi-src
Copy link
Contributor Author

varungandhi-src commented Oct 12, 2022

Looks like I just reimplemented that PR but in a different function. 🤦🏽

I thought Yarn Berry was some different fork of Yarn or something that we added special support for.

@olafurpg
Copy link
Contributor

cc/ @valerybugakov any opinions about having a separate flag for Yarn v3?

@valerybugakov
Copy link
Member

@olafurpg, I don't see a reason for having a separate flag for Yarn v3 if we already have --yarn-berry-workspaces. There's the next major yarn version on the horizon, so it makes sense to bind the flag to the package-manager workspaces protocol, not the package-manager version.

@olafurpg
Copy link
Contributor

@valerybugakov I meant, should we aim to consolidate --yarn-workspaces and --yarn-berry-workspaces? Varun implemented this PR without realizing we already support v3 because the --yarn-berry-workspaces name is not self-explanatory for somebody who is not intimately familiar with Yarn development

@valerybugakov
Copy link
Member

should we aim to consolidate --yarn-workspaces and --yarn-berry-workspaces?

I see. Then yes, hiding this detail from the user would be nice 👍

@varungandhi-src
Copy link
Contributor Author

OK, looks like we're in agreement then. I've removed the code duplication between the newly added implementation and the previously added one.

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

@varungandhi-src varungandhi-src merged commit 9f5d08a into main Oct 13, 2022
@varungandhi-src varungandhi-src deleted the vg/fix-yarn3 branch October 13, 2022 03:14
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.

scip-typescript fails to index Yarn workspaces with Yarn 3

4 participants