Adds a failing test for focused workspaces

[arcanis]

While working on making the workspace root implicitly depend on all of its child workspaces, I noticed a bug in the current implementation of the focused workspace feature. If the workspace root depend on a workspace, it won't get focused (symlinks will always be used instead).

It might also occur even for sibling workspaces, I just haven't checked yet. The workspace root is the one I'm the most interested in making it work for now.

This diff adds a test that reproduce the issue. @bdwain, do you have some bandwidth to give it a look? It would be super helpful 😃

[bdwain]

@arcanis yea i can look tonight. Thanks for finding it.

What exactly does it mean for a workspace root to depend on a workspace? Is that a shortcut to have every other package depend on it?

Also what do you mean it might even be for sibling workspaces?

[arcanis]

What exactly does it mean for a workspace root to depend on a workspace? Is that a shortcut to have every other package depend on it?

No, it just means that if / has a dependency on bar via the workspace /packages/bar, then files located in (for example) /scripts are guaranteed to be able to require files from the bar package (otherwise /node_modules/bar is not guaranteed to exist). You can think of the workspace root as a separate workspace on top of the ones you define: it can have its own dependencies.

It currently doesn't implicitly depend on its child workspaces, so the symlinks are not guaranteed to exist by default. It's kinda annoying since workspace roots might want to use tools provided by their workspaces (for example, the workspace root of the Jest project would have to list the jest package as dependency in order to be able to use the jest binary). My change makes this implicit in such a way that if a workspace root has a workspace, then it depends on it by default (so jest would be available).

Also what do you mean it might even be for sibling workspaces?

Let's say you have Foo, Bar, and Baz. Foo and Bar depend on Baz.

I wonder what would happen if you were to focus on Foo. Would Bar also contain the materialized Baz package, or would it be a symlink to the Baz workspace?

[bdwain]

Ah got it. I'll look into that all tonight. For the sibling thing, the intended behavior was for focus to only affect the focused workspace, so Bar would be relying on the symlink at root/node_modules/Baz and not a physical copy downloaded from the registry. I'm pretty sure it's behaving as intended, but I can double check.

[bdwain]

It seems to be related to the fact that when the root project depends on example workspace 1, the hoist manifest for workspace 2 has a previous path that is missing a slash.

Still looking into why, but the second one should start with a slash and it doesn't, which is causing a check to fail. I'll look into why, as I'm not yet familiar with that part of the code.

[bdwain]

@arcanis I think I have a fix. It's like one line so it might be easier to just throw it in your PR.

in package-hoister, NoHoistResolver._makePath should be changed to

  _makePath(...args: Array<string>): string {
    const parts = args.map(s => (s === this._wsRootPackageName ? WS_ROOT_ALIAS : s));
    const result = parts.join('/');
    return result[0] === '/' ? result : '/' + result;
  }

from

  _makePath(...args: Array<string>): string {
    const parts = args.map(s => (s === this._wsRootPackageName ? WS_ROOT_ALIAS : s));
    return parts.join('/');
  }

That seems to fix the issue because we have a check at some point saying

      const searchPath = `/${WS_ROOT_ALIAS}/${parentName}`;
      info = this.tree.get(workspace);
      invariant(info, 'missing workspace tree entry ' + workspace);
      if (!info.previousPaths.some(p => p.startsWith(searchPath))) {
        return;
      }

and if the previousPath does not begin with /, it will fail.

I also added a few comments on the test you created.

Also, the sibling behavior seems correct.

[bdwain]

this check will always fail because it's expecting it to not be a symlink even though it is one.

[arcanis]

Why should it be a symlink? If you focus on a workspace I thought it meant that all other workspaces would be fetched from the network.

[bdwain]

The implementation is actually a little different from that. When you do a focused install, it installs everything like normal, and only adds in remote installs "shallowly". That means it doesn't put the remote copy of example-yarn-workspace-2 at root/node_modules. That is still a symlink to the local copy. Instead it installs it at packages/example-yarn-workspace-1/node_modules/examples-yarn-workspace-2. That helps deal with version conflict issues and also makes it quicker to switch back and forth between focused and not focused.

[bdwain]

so basically the shallow install comes first in node module resolution so it masks the symlink and causes it not to be used.

[bdwain]

there definitely should be one at this path, so I would recommend removing this check.

[arcanis]

Ideally there shouldn't, because it should be hoisted to the root (since both the root and example-yarn-workspace-1 share the same dependency to example-yarn-workspace-2).

[bdwain]

see my comment above. It would never hoist the remote copy of the workspace.

[bdwain]

oh i see what you're trying to do. You want to allow the stuff in the root workspace to depend on the pre-built copy of example-yarn-workspace-2.

Making something like that work would be a much bigger change I think. Because focus was only intended to affect the workspace you focus on. So the root workspace was never really intended to be affected (and neither were other workspaces). Also I didn't realize the root workspace was a regular workspace that could be used but that's a separate thing.

[bdwain]

existsAndIsMaterialized has no return value, so this always fails. You can just get rid of the expect, since existsAndIsMaterialized makes the proper check inside of itself

[arcanis]

👍

[arcanis]

@bdwain Thanks for the fix, it seems to work. I'm merging it as part of #6382, and will merge this PR after that.