Focused Workspaces RFC Implementation #5663

bdwain · 2018-04-13T06:54:19Z

UPDATE: This is complete and ready for final review.

I've been working on an implementation of the focused workspaces RFC. It's not complete yet, but it mostly works so I was hoping to get some feedback as i try to get the last few things working to make sure my implementation makes sense.

All of the details of what it does and the edge cases are in the RFC link above, so I won't explain any of that here. I was just hoping to get general high level feedback on my approach. Most importantly, I was hoping for feedback on was how I used the hoisted tree result (from package-hoister) to calculate which packages need to be shallowly installed inside the target workspace and where. I'll comment inline to point out where that is.

FYI I haven't written any unit tests yet. I'm beginning work on those now. But I wanted to post this early to get feedback in case there are any major issues with my implementation.

Sorry in advance if there's a better place to do this (like a development chat or something). This is my first big yarn change so I wasn't sure what the normal process was.

Thanks!

cc @bestander

bdwain · 2018-04-13T06:56:07Z

src/package-request.js

    const exoticResolver = getExoticResolver(this.pattern);
    if (exoticResolver) {
      return this.findExoticVersionInfo(exoticResolver, this.pattern);
    } else if (WorkspaceResolver.isWorkspace(this.pattern, this.resolver.workspaceLayout)) {
      invariant(this.resolver.workspaceLayout, 'expected workspaceLayout');
      const resolver = new WorkspaceResolver(this, this.pattern, this.resolver.workspaceLayout);
-      return resolver.resolve();
+      let manifest;


workspaces were never downloaded from the registry before before, so i had to add that here.

buildsize · 2018-04-13T06:56:54Z

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	913.56 KB	917.59 KB	4.03 KB (0%)
yarn-[version].js	3.96 MB	3.97 MB	14.06 KB (0%)
yarn-legacy-[version].js	4.12 MB	4.13 MB	16.84 KB (0%)
yarn-v[version].tar.gz	918.53 KB	922.63 KB	4.09 KB (0%)
yarn_[version]all.deb	678.72 KB	681.36 KB	2.64 KB (0%)

bdwain · 2018-04-13T07:02:18Z

src/package-hoister.js

@@ -649,6 +658,81 @@ export default class PackageHoister {
    }
  }

+  markShallowWorkspaceEntries(){


this is where the main logic is happening.

bdwain · 2018-04-13T07:02:46Z

src/package-hoister.js

+        if(splitPath[0] !== w){ //entry is not related to the workspace
+          return false;
+        }
+        if(!splitPath[1]){ //entry is the workspace


all sibling workspaces are shallow dependencies

bdwain · 2018-04-13T07:03:31Z

src/package-hoister.js

+        if(!splitPath[1]){ //entry is the workspace
+          return true;
+        }
+        //don't bother marking dev dependencies for shallow installation


Looks for any dependencies the sibling workspaces already install shallowly (due to version conflicts with the hoisted version) and marks those for shallow installation.

bdwain · 2018-04-13T07:04:18Z

src/package-hoister.js

+        info.shallowPaths = [''];
+        return;
+      }
+


Finally, it looks for packages that are normally hoisted but need to be shallowly installed underneath a sibling workspace because the target has its own version, and marks those for shallow installation (but not just directly under node_modules like the other ones)

bdwain · 2018-04-13T07:08:54Z

src/package-hoister.js

      if (this.config.modulesFolder) {
        // remove the first part which will be the folder name and replace it with a
        // hardcoded modules folder
        parts.splice(0, 1, this.config.modulesFolder);
      } else {
        // first part will be the registry-specific module folder
        parts.splice(0, 0, this.config.lockfileFolder);
+        if(info.shallowPaths){


add any packages marked for shallow installation to the main output tree

bdwain · 2018-04-15T09:17:18Z

I updated my original comment because I addressed a bunch of the incomplete things I originally mentioned.

bdwain · 2018-04-15T09:21:36Z

src/config.js

@@ -399,14 +399,10 @@ export default class Config {
   * Generate an absolute module path.
   */

-  generateHardModulePath(pkg: ?PackageReference, ignoreLocation?: ?boolean): string {
+  generateModuleCachePath(pkg: ?PackageReference): string {


this was kind of being used for 2 different purposes. I removed the part about getting the path to node_modules because that was kind of broken anyway (A package can be installed to more than one location). Instead the code that relied on location being set uses the packagereference directly.

bdwain · 2018-04-15T09:25:51Z

src/package-reference.js

  resolver: PackageResolver;

  setFresh(fresh: boolean) {
    this.fresh = fresh;
  }

-  setLocation(loc: string): string {
-    return (this.location = loc);
+  addLocation(loc: string): string {


a package can have more than one location. For example, 3 workspaces depend on foo@2.0.0, and 2 workspaces depend on foo@1.0.0. foo@1.0.0 will be installed under both workspaces to avoid the conflict with the hoisted v2.

This was causing a bug where .bin links pointing to foo@1.0.0 would all point to a single copy of it, rather than each one pointing to the one installed for that package (the one it will actually use).

I also think if foo@1.0.0 has a postinstall script, it would only have run on one instance, but I never verified that. It shouldn't do that either way now though.

bdwain · 2018-04-15T09:27:34Z

src/resolvers/contextual/workspace-resolver.js

+      registryRemote = downloadedManifest._remote;
+      hash = registryRemote.hash;
+      //override any local changes to manifest
+      Object.keys(manifest).forEach(k => k.startsWith('_') || delete manifest[k]);


this was to avoid weird bugs where the local package.json differed from the remote one (for the same version).

bdwain · 2018-04-20T21:14:04Z

@bestander any thoughts on this implementation? (Or do you have anyone else you think should weigh in?). I'm getting close to having something that is complete and would like to know sooner rather than later if there are any major implementation issues.

Thanks!

bestander · 2018-04-21T09:42:22Z

@bdwain, great job taking it to a working solution!

Half of it is a no-brainer, half of it need thinking like changing package-reference interface.

I’ve scrolled through the code, are all these changes necessary for the feature or is it some refactoring that could be on its own?

bdwain · 2018-04-21T16:37:54Z

Thanks @bestander. I think all of the changes are necessary. They were all done to fix one edge case or another. Can you comment on the specific ones you’re concerned about? I'd be happy to explain any of them.

For the package reference interface, I changed it to multiple locations because without that, the top level symlink of a workspace and the shallow installation would have the same location, and so any .bin links pointing to that package would both point to the location (which ended up being the one whose path ended up last alphabetically). This also caused similar issues with not running post install scripts on all of the installations.

this also fixed what I feel was incorrect behavior from before in the scenario where a package was installed in multiple locations (if it couldn’t be hoisted due to another version being hoisted). In that case, all of the unhoisted versions would point to the same location, so for example, workspace b’s bin link for foo might point to workspace a’s installation of foo becuse they had the same location. And similar issues for running all post install scripts.

I looked around and feel pretty confident that the change caused no unintended side effects. But if I misssd one id be happy to address it. But I do think the change is needed now since focused workspaces make it more likely to have multiple installations of a package and for those installations to be different.

bdwain · 2018-04-21T19:46:39Z

package.json

@@ -117,6 +117,7 @@
    "release-branch": "./scripts/release-branch.sh",
    "test": "yarn lint && yarn test-only",
    "test-only": "node --max_old_space_size=4096 node_modules/jest/bin/jest.js --verbose",
+    "test-only-debug": "node --inspect-brk --max_old_space_size=4096 node_modules/jest/bin/jest.js --runInBand --verbose",


i can remove this if needed but i found it very useful

Looks all right to me

bdwain · 2018-04-21T19:50:35Z

src/resolvers/contextual/workspace-resolver.js

+      registryRemote = downloadedManifest._remote;
+      invariant(registryRemote, 'missing remote info');
+      hash = registryRemote.hash;
+      //override any local changes to manifest


this is related to the comment here.

bdwain · 2018-04-21T20:48:10Z

also FYI i have all old unit tests passing, so besides addressing any PR comments, the only thing left is to add in new unit tests for the focus functionality

bdwain · 2018-04-22T22:36:19Z

I think I'm going to need to create a test repo with some workspaces in it for some new unit tests. I'll create it under my account for now, but would it make sense to move to the yarnpkg account so other people can edit it easily in the future if needed?

bdwain · 2018-04-24T07:17:24Z

__tests__/commands/check.js

@@ -33,6 +33,16 @@ test.concurrent('--verify-tree should report wrong version', async (): Promise<v
  expect(thrown).toEqual(true);
 });

+test.concurrent('--verify-tree should work from a workspace cwd', async (): Promise<void> => {


i made the change in check.js so that this test would pass. I needed it to work correctly for the new focus tests to work (or I could have avoided the check at the end of the install and be different from other install tests, but that felt wrong)

bestander · 2018-04-29T06:19:49Z

Looks reasonable so far.
A bit more work than I expected :) but I appreciate the effort and dedication you make.
Ping me when you are done with tests.

bdwain · 2018-04-29T22:34:37Z

@bestander i think this is just about ready to go. The unit tests are all there. I did also make some changes to the main logic to fix some edge cases I missed in this commit if you want to see the differences.

I am seeing one weird issue where on the node 4 CI builds, i get some errors trying to use es6 methods like Array.includes, but i notice instances of it being used in the same file that do not cause issues, which is confusing. I'm going to switch it to use indexOf for now, but it seems like i should not need to.

EDIT: Ignore that last thing. idk if it was actually used anywhere. Are babel-polyfill methods just not available to use for yarn?

I also am a bit confused on one last test that's failing only in the appveyor CI build, which seems to run on windows. It's my new .bin link test, but it passes on my local machine (a mac). I don't see anything different from the others. I'll keep looking to see why, but if you have any suggestions I'd appreciate it. It's a slow debugging process since I need to commit to test it and wait for CI.
EDIT: Can i not set a different cwd than the default for these .bin link tests? That's one difference i notice from the others.
It was that my script was missing a #!/usr/bin/env node at the top of my script

~~Other than that one failing test, I think that's it.~~

Thanks!

bdwain · 2018-04-30T01:36:25Z

@bestander the issues i mentioned above are fixed. So I've got nothing left to do except address PR comments.

bdwain · 2018-04-30T02:07:15Z

also if it's helpful, i can transfer my example repo and the associated npm packages (which I needed to make for the tests I added) to yarn so people can edit them if needed in the future.

bestander · 2018-05-05T16:17:36Z

Thanks Bryan. Sorry I’ve been phased out for a week, will review shortly

…

On Sun, Apr 29, 2018 at 7:07 PM Bryan Wain ***@***.***> wrote: also if it's helpful, i can transfer my example repo <https://github.com/bdwain/example-yarn-workspaces> and the associated npm packages (which I needed to make for the tests I added) to yarn so people can edit them if needed in the future. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5663 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWIe3GMQ6JbQcHvIx_DSFYLtqLhDCks5ttnHdgaJpZM4TS7__> .

bestander

Awesome work, @bdwain!
Thank you for your dedication and the effort you've made to add tests and consider the edge cases.

I don't have any concerns about this.
Here is what we do next:

Wait for @arcanis approval to make sure this feature doesn't interfere with any features already in the works
Merge this PR
We'll need a blogpost https://yarnpkg.com/blog/ about this new feature! :)

bdwain · 2018-05-06T22:25:23Z

Sounds great! Thanks! I can work on a blog post. Should I just make a pr to https://github.com/yarnpkg/website/tree/master/_posts ?

One other question is what will the process be to release this once all 3 of those steps are done? It's not a breaking change so can it just go out in v 1.7 or whatever the next minor release is?

arcanis · 2018-05-09T22:15:31Z

Hey @bdwain! Thanks for submitting the PR! I'll look at it tomorrow and come back to you 👍

arcanis · 2018-05-09T22:19:01Z

Sounds great! Thanks! I can work on a blog post. Should I just make a pr to https://github.com/yarnpkg/website/tree/master/_posts ?

Yep, that would be perfect!

One other question is what will the process be to release this once all 3 of those steps are done? It's not a breaking change so can it just go out in v 1.7 or whatever the next minor release is?

If there's no breaking change it should be fine in a minor :)

bdwain · 2018-05-16T03:33:53Z

Hi @arcanis Please let me know if there's anything else you need before you look at this. I was holding off on finalizing the blog post until it was merged because if anything changes I didn't want to have to change it.

Thanks!

bestander · 2018-05-17T17:42:46Z

@bdwain, great job on this, sorry for a delay, we are all heads down working on specific projects.
Let me take the responsibility and merge it, I don't expect anyone to be extremely opposed to the changes.
Clicking merge button!

bestander · 2018-05-17T17:43:09Z

Is blog post ready? I'll be happy to spread the word

bdwain · 2018-05-17T18:00:27Z

thanks! It's not ready yet. I'll try to have it ready in the next day or so.

arcanis · 2018-06-11T14:37:37Z

Noticed what I think is a bug, I opened #5967 to discuss it 🙂

brownbl1 · 2018-10-28T00:33:43Z

@bdwain Will a blog post for this still be written or has this stalled? I'm having a hard time discerning whether this has actually been released/published or not. Thanks!

bdwain · 2018-10-28T01:37:58Z

yup it was released already! https://yarnpkg.com/blog/2018/05/18/focused-workspaces/

brownbl1 · 2018-10-28T01:59:55Z

Thanks for the link! That's helpful. Now I see that this feature didn't take the path I was looking for. I was hoping for something more along the lines of #4207. In my case, I'm not looking to install packages from the registry. I just want a trimmed down node_modules that applies only to my package (and copies down the dependencies that are in the monorepo).

My reasoning is that I have an app that is in a monorepo. But on the CI server, we build it and need to zip it up along with it's node_modules folder to publish. I don't want to just take the top level node_modules folder since it has tons of extraneous stuff in it, but I also don't want to break the monorepo pattern too much either and still want to take advantage of the top level yarn.lock. Maybe I'm doing it wrong.. but it seemed like some of the other approaches/issues would have worked for me.

👍

bdwain · 2018-10-28T02:04:37Z

I see. I think focused workspaces was attempting to solve a different problem. But yarn plug'n'play seems like it may help you solve your issues.

RDeluxe · 2018-11-26T11:16:07Z

I'm in the same boat, I thought this could solve our problems with package isolation while creating docker images for example.

bdwain commented Apr 13, 2018

View reviewed changes

bdwain commented Apr 15, 2018

View reviewed changes

bdwain force-pushed the isolated-v3 branch 2 times, most recently from d98bc27 to 9bb3101 Compare April 18, 2018 03:34

bdwain force-pushed the isolated-v3 branch from 9bb3101 to 9945919 Compare April 21, 2018 04:48

bdwain commented Apr 21, 2018

View reviewed changes

bdwain changed the title ~~Request for feedback on Focused Workspaces RFC Implementation~~ Focused Workspaces RFC Implementation Apr 21, 2018

bdwain force-pushed the isolated-v3 branch from 7bbec7a to 56445d9 Compare April 24, 2018 06:48

bdwain commented Apr 24, 2018

View reviewed changes

bdwain force-pushed the isolated-v3 branch from 7d578ed to b03e5f0 Compare April 27, 2018 23:35

bdwain force-pushed the isolated-v3 branch 2 times, most recently from c6df2ae to c5a07cf Compare April 29, 2018 22:30

bdwain force-pushed the isolated-v3 branch from c5a07cf to c7c0e54 Compare April 29, 2018 22:39

bdwain force-pushed the isolated-v3 branch 2 times, most recently from 74f7c4f to 69c7639 Compare April 30, 2018 01:35

bdwain force-pushed the isolated-v3 branch 2 times, most recently from 9665041 to c3e674f Compare May 4, 2018 01:52

bdwain force-pushed the isolated-v3 branch from c3e674f to 31b9738 Compare May 6, 2018 18:20

bestander approved these changes May 6, 2018

View reviewed changes

bdwain force-pushed the isolated-v3 branch 2 times, most recently from d61f100 to 4c10f09 Compare May 14, 2018 04:10

focused workspaces

ddaac18

bdwain force-pushed the isolated-v3 branch from 4c10f09 to ddaac18 Compare May 17, 2018 14:22

bestander merged commit cde7566 into yarnpkg:master May 17, 2018

bdwain deleted the isolated-v3 branch May 17, 2018 18:00

spalger mentioned this pull request May 24, 2018

TypeError: Reduce of empty array with no initial value #5876

Closed

bdwain mentioned this pull request May 25, 2018

feature: workspaces: ability to install in a workspace in isolation #4099

Open

Focused Workspaces RFC Implementation #5663

Focused Workspaces RFC Implementation #5663

Conversation

bdwain commented Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

buildsize bot commented Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdwain commented Apr 15, 2018

Choose a reason for hiding this comment

bdwain Apr 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdwain commented Apr 20, 2018

bestander commented Apr 21, 2018

bdwain commented Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdwain commented Apr 21, 2018

bdwain commented Apr 22, 2018

bdwain Apr 24, 2018 • edited Loading

Choose a reason for hiding this comment

bestander commented Apr 29, 2018

bdwain commented Apr 29, 2018 • edited Loading

bdwain commented Apr 30, 2018

bdwain commented Apr 30, 2018

bestander commented May 5, 2018 via email

bestander left a comment

Choose a reason for hiding this comment

bdwain commented May 6, 2018

arcanis commented May 9, 2018

arcanis commented May 9, 2018

bdwain commented May 16, 2018

bestander commented May 17, 2018

bestander commented May 17, 2018

bdwain commented May 17, 2018

arcanis commented Jun 11, 2018

brownbl1 commented Oct 28, 2018

bdwain commented Oct 28, 2018

brownbl1 commented Oct 28, 2018

bdwain commented Oct 28, 2018

RDeluxe commented Nov 26, 2018

bdwain commented Apr 13, 2018 •

edited

Loading

buildsize bot commented Apr 13, 2018 •

edited

Loading

bdwain Apr 15, 2018 •

edited

Loading

bdwain commented Apr 21, 2018 •

edited

Loading

bdwain Apr 24, 2018 •

edited

Loading

bdwain commented Apr 29, 2018 •

edited

Loading