feat: proper yarn workspace support #379

nlunets · 2019-07-26T08:57:34Z

Fixes #300

This PR changes the way yarn bundling is being done.
The main thing to consider is that yarn list is nowhere close to what npm ls does so you cannot really rely on that for bundling, especially in a workspace context.

Instead the code goes through the dependency based on the node resolution algorithm https://nodejs.org/api/modules.html#modules_all_together

At package time this means we might end up with path like "../../../node_modules" which obviously do not make sense in a packaged environment, so those path are stripped to just be node_modules ...

msftclas · 2019-07-26T08:57:48Z

All CLA requirements met.

joaomoreno · 2019-07-26T12:23:30Z

I'm also not too excited to simply duplicate npm's internal behavior. Any chance we can simply depend on npm and use its API? #246

nlunets · 2019-07-26T12:41:29Z

Unfortunately not really, running npm-ls on a yarn workspace installed package just results in a lot of errors because the package are not where they are expected.

Furthermore we cannot even rely on the basic node resolution capabilities (require.resolve and require.resolve.paths).
The first one expect a resolveable package (ie: with a clear main script) which is not a guarantee (see babel/babel#5509)
The second one works based on the current file location (ie the npm.js file) which could be located anywhere in the dependency tree.

From the issue you linked it seems that even npm ls is problematic anyway...
In the end what you want to bundle is not related to who installed it or how they installed it, even npm or yarn can install packages in many different ways and your local setup might affect that.
Following the node resolve logic is (in my opinion) a safer approach as you would now depend on only one logic, which has been stable for years and which works regardless of which package manager installed the dependencies or how they did it :/

nlunets · 2019-08-28T11:32:44Z

@joaomoreno any chance for you to have a deeper review on this ?

brpowell · 2019-08-28T16:53:10Z

@joaomoreno any chance for you to have a deeper review on this ?

Would love to see this reviewed! The change would unblock adoption of yarn workspaces for our project.

joaomoreno · 2019-11-28T16:39:01Z

@nlunets Looking into this. Can you give me a concrete example of an extension which would depend on this?

Also, would it be enough to simply detect symlinks and resolve them accordingly, instead of changing the whole algorithm?

ullasholla · 2019-12-10T22:29:44Z

@joaomoreno, here's a example repo you can test against: https://github.com/ullasholla/yarn-workspace-test

yarn hoists dependencies to the workspace root and hence just following symlinks will not help.

ullasholla · 2020-01-07T19:00:41Z

@joaomoreno, have you had a chance to look at this? Cheers.

bestander · 2020-01-15T04:06:27Z

Thanks @nlunets for sending the patch.
Until the PR is accepted I've published the patch to npm https://www.npmjs.com/package/vsce-yarn-patch

bestander · 2020-01-15T04:08:08Z

And I take some(ok, all) responsibility that yarn ls command sucks in workspaces mode :(
It just slipped through the cracks.
Now it's a bit too late with Yarn V2 being imminent.

@joaomoreno, any chance this can be accepted?
The fix does work on workspaces repos.

joaomoreno

And I take some(ok, all) responsibility that yarn ls command sucks in workspaces mode :(
It just slipped through the cracks.
Now it's a bit too late with Yarn V2 being imminent.

What do you mean?

joaomoreno · 2020-01-15T09:53:58Z

src/package.ts

+		const files = fileNames.map(f => {
+			// In case of a yarn workspace the filepath can point to node_modules at a higher level, we can just strip that within the extension
+			let basePathF = f;
+			while(basePathF.indexOf('../') === 0) {


Did you test this on Windows? Also, please do not use indexOf to check for a prefix...

joaomoreno · 2020-01-15T09:55:08Z

src/npm.ts

+	let dependencyPath;
+	let newPrefix = prefix;
+	// Follow the same resolve logic that is used within npm / yarn
+	while(newPrefix !== "/" && !dependencyPath) {


Works on Windows?

joaomoreno · 2020-01-15T09:56:03Z

src/npm.ts

+	if(!dependencyPath) {
+		dependencyPath = path.join(prefix, "node_modules", name)
+	}
+	const depPackage = require(path.join(dependencyPath, "package.json"));


Do not use require to load a JSON file from disk.

joaomoreno · 2020-01-15T09:58:20Z

src/npm.ts

 	const usingPackagedDependencies = Array.isArray(packagedDependencies);
-	const trees = JSON.parse(match[0]).data.trees as YarnTreeNode[];
+	const rootPackage = require(path.join(cwd, 'package.json'));


Do not use require to load a JSON file from disk.

joaomoreno · 2020-01-15T09:58:55Z

src/npm.ts

 	const usingPackagedDependencies = Array.isArray(packagedDependencies);
-	const trees = JSON.parse(match[0]).data.trees as YarnTreeNode[];
+	const rootPackage = require(path.join(cwd, 'package.json'));
+	const trees = Object.keys(rootPackage.dependencies);


Instead of trees, shouldn't it be packages? Or dependencies?

joaomoreno · 2020-01-15T10:00:08Z

src/package.ts

@@ -837,7 +837,14 @@ export function collect(manifest: Manifest, options: IPackageOptions = {}): Prom
 	const processors = createDefaultProcessors(manifest, options);

 	return collectFiles(cwd, useYarn, packagedDependencies).then(fileNames => {
-		const files = fileNames.map(f => ({ path: `extension/${f}`, localPath: path.join(cwd, f) }));
+		const files = fileNames.map(f => {


I'd rather we change the return values of collectFiles to return absolute paths instead of hardcoding yarn workspaces knowledge down here.

bestander · 2020-01-15T19:21:04Z

And I take some(ok, all) responsibility that yarn ls command sucks in workspaces mode :(
It just slipped through the cracks.
Now it's a bit too late with Yarn V2 being imminent.

What do you mean?

Never mind, I implemented the workspaces in yarn and did not realize that list command got broken.
It's worth making a fix in Yarn V2 https://github.com/yarnpkg/berry though when it's released

bestander · 2020-01-15T19:36:55Z

I was a bit too eager on this PR, turns out it goes into infinite loop in some cases.
It needs to maintain a set of visited folders when looping through node_modules.

bestander · 2020-01-15T20:01:35Z

@nlunets, I've forked your PR and pushed this fix to the infinite loop bestander@44d3c26

Let us know if you want to proceed with the PR feedback

nlunets · 2020-01-17T14:40:52Z

@bestander thanks for your involvement !
Since you probably have deeper knowledge of how yarn does its magic it might indeed be better if you take care of the PR feedback !
Thanks !

bestander · 2020-01-17T17:47:04Z

Thanks, I'll resubmit the PR on weekend

…

On Fri, 17 Jan 2020 at 06:40, Nicolas Lunet ***@***.***> wrote: @bestander <https://github.com/bestander> thanks for your involvement ! Since you probably have deeper knowledge of how yarn does its magic it might indeed be better if you take care of the PR feedback ! Thanks ! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#379?email_source=notifications&email_token=AAQF2WD2EHKZQ2XNEUS22DLQ6G7PLA5CNFSM4IHB3Q52YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJH4HJY#issuecomment-575652775>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAQF2WCXUZZZRVDI5PNTW7TQ6G7PLANCNFSM4IHB3Q5Q> .

bestander · 2020-02-06T07:40:38Z

Apologies, everyone.
I was distracted by other work and did not finish cleaning up the PR.
I plan to follow up but can't commit to a specific date yet.

alexgorbatchev · 2020-04-24T19:25:21Z

Ping! Would love to see this merged soon! Thanks for making it happen!

joaomoreno · 2020-04-27T13:51:16Z

Closing since the feedback wasn't addressed and it seems @bestander will simply create a new one.

feat: proper yarn workspace support

070e81a

joaomoreno self-assigned this Jul 26, 2019

joaomoreno added this to the Backlog milestone Jul 26, 2019

joaomoreno removed this from the Backlog milestone Oct 11, 2019

joaomoreno requested changes Jan 15, 2020

View reviewed changes

joaomoreno closed this Apr 27, 2020

sebas2day mentioned this pull request Jun 22, 2020

feat: Support for yarn workspaces #458

Closed

sebas2day mentioned this pull request Sep 29, 2020

feat: Support for yarn workspaces #493

Merged

karlhorky mentioned this pull request Jan 2, 2021

docs: fix vscode-vsce PR details yarnpkg/berry#2314

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: proper yarn workspace support #379

feat: proper yarn workspace support #379

nlunets commented Jul 26, 2019

msftclas commented Jul 26, 2019 •

edited

Loading

joaomoreno commented Jul 26, 2019

nlunets commented Jul 26, 2019 •

edited

Loading

nlunets commented Aug 28, 2019

brpowell commented Aug 28, 2019

joaomoreno commented Nov 28, 2019 •

edited

Loading

ullasholla commented Dec 10, 2019

ullasholla commented Jan 7, 2020

bestander commented Jan 15, 2020

bestander commented Jan 15, 2020 •

edited

Loading

joaomoreno left a comment

joaomoreno Jan 15, 2020

joaomoreno Jan 15, 2020

joaomoreno Jan 15, 2020

joaomoreno Jan 15, 2020

joaomoreno Jan 15, 2020

joaomoreno Jan 15, 2020

bestander commented Jan 15, 2020

bestander commented Jan 15, 2020

bestander commented Jan 15, 2020

nlunets commented Jan 17, 2020

bestander commented Jan 17, 2020 via email

bestander commented Feb 6, 2020

alexgorbatchev commented Apr 24, 2020

joaomoreno commented Apr 27, 2020

feat: proper yarn workspace support #379

feat: proper yarn workspace support #379

Conversation

nlunets commented Jul 26, 2019

msftclas commented Jul 26, 2019 • edited Loading

joaomoreno commented Jul 26, 2019

nlunets commented Jul 26, 2019 • edited Loading

nlunets commented Aug 28, 2019

brpowell commented Aug 28, 2019

joaomoreno commented Nov 28, 2019 • edited Loading

ullasholla commented Dec 10, 2019

ullasholla commented Jan 7, 2020

bestander commented Jan 15, 2020

bestander commented Jan 15, 2020 • edited Loading

joaomoreno left a comment

Choose a reason for hiding this comment

joaomoreno Jan 15, 2020

Choose a reason for hiding this comment

joaomoreno Jan 15, 2020

Choose a reason for hiding this comment

joaomoreno Jan 15, 2020

Choose a reason for hiding this comment

joaomoreno Jan 15, 2020

Choose a reason for hiding this comment

joaomoreno Jan 15, 2020

Choose a reason for hiding this comment

joaomoreno Jan 15, 2020

Choose a reason for hiding this comment

bestander commented Jan 15, 2020

bestander commented Jan 15, 2020

bestander commented Jan 15, 2020

nlunets commented Jan 17, 2020

bestander commented Jan 17, 2020 via email

bestander commented Feb 6, 2020

alexgorbatchev commented Apr 24, 2020

joaomoreno commented Apr 27, 2020

msftclas commented Jul 26, 2019 •

edited

Loading

nlunets commented Jul 26, 2019 •

edited

Loading

joaomoreno commented Nov 28, 2019 •

edited

Loading

bestander commented Jan 15, 2020 •

edited

Loading