Workspaces phase 2: executing commands #3365

arcanis · 2017-05-10T14:16:59Z

The current implementation allows to run a command inside the context of a workspace, regardless where the user is inside the project:

$> yarn workspace babel-runtime add babel-core

arcanis · 2017-05-10T14:19:03Z

There might be flow / lint issues, I'll fix them before merging.

bestander · 2017-05-10T14:19:47Z

Awesome!
Will look at it later today

bestander · 2017-05-10T14:21:08Z

How about some tests?
See new test file in #3229

arcanis · 2017-05-10T14:36:34Z

Will git see that these are the same files even if on different branches?

bestander · 2017-05-10T16:21:13Z

no, you'll probably need to rebase and resolve conflicts after my branch is merged

bestander · 2017-05-10T16:21:40Z

but you can probably rebase on top of my branch

arcanis · 2017-05-10T16:25:38Z

If I do this it'll cause issues if you rebase yours later :/ I'll try to add a commit with your fixtures, and remove it before merging if git doesn't like it.

bestander · 2017-05-11T14:40:26Z

src/cli/commands/workspace.js

+  }
+
+  try {
+    await child.spawn(process.argv[0], [process.argv[1], ... rest], { stdio: 'inherit' });


Smart move :)
But I don't think we want to necessarily shell out to start another node VM.
I would prefer us doing it via requires.

There's quite a bit of logic that we currently can't run in the same process (most notably, everything related to the command line parsing, which is run in src/cli/index.js). That could be refactored, but I felt it could be too heavy for prototyping the command.

Got it, I think workspace command won't interfere with regular workflows, looks fine then

arcanis · 2017-05-15T15:55:29Z

@bestander I've just rebased on master (ie on your merged PR), and added a commit that implements a project detection logic inside install (which is then used by add & remove). Could you review the implementation, except on the following points?

I need to avoid computing the project path multiple times. It's really bad for performances. Ideally, it should only be computed once. Two solutions: either it is computed in the Config.init() function, or I can add a cache to findProject. I like the first solution better.
I need to write some tests to make sure everything's acting correctly, but the manual tests I've made seem to indicate that everything looks right.
I need to blacklist node_modules inside resolveWorkspaces, otherwise we might end up picking up the package.json files of our dependencies ... not cool 😄

bestander · 2017-05-16T22:14:21Z

src/cli/commands/add.js

@@ -204,7 +204,7 @@ export async function run(
    throw new MessageError(reporter.lang('missingAddDependencies'));
  }

-  const lockfile = await Lockfile.fromDirectory(config.cwd, reporter);
+  const lockfile = await Lockfile.fromDirectory(config, config.cwd, reporter);


config.cwd looks redundant here, do we ever need it not from config?

Not really, but it makes sense semantically to specify the path, otherwise it should be renamed to fromConfig - and as you mentioned in the comment below, it would probably be for the best to keep this function not too tightly coupled with the config object.

In that case would it be better to pass no the full config to the fromDirectory function?
Technically you need only the config.worktreeFolder || config.cwd and is dir needed then?

bestander

Looks quite simple, so let's do some e2e tests

bestander · 2017-05-16T22:24:17Z

src/lockfile/wrapper.js

@@ -86,9 +87,12 @@ export default class Lockfile {
    [key: string]: LockManifest
  };

-  static async fromDirectory(dir: string, reporter?: Reporter): Promise<Lockfile> {
+  static async fromDirectory(config: Config, dir: string, reporter?: Reporter): Promise<Lockfile> {


nit: this couples lockfile wrapper to config, not sure if it is a concern but maybe we'll want to decouple it back in the future

Yep, it should be fairly easy. I think most tools currently stored inside the config object (such as tryManifest, etc) could be safely moved somewhere else (for example in miniyarn, I had a util/yarn file to store all such functions), so that the config would only contain the configuration data and nothing else.

bestander · 2017-05-16T22:40:53Z

src/cli/commands/workspace.js

+  }
+
+  try {
+    await child.spawn(process.argv[0], [process.argv[1], ... rest], { stdio: 'inherit' });


Got it, I think workspace command won't interfere with regular workflows, looks fine then

bestander · 2017-05-16T22:41:11Z

src/cli/commands/workspace.js

+
+  try {
+    await child.spawn(process.argv[0], [process.argv[1], ... rest], { stdio: 'inherit' });
+  } catch (err) {


I think you don't want to gobble up the error here?

The issue I had was that regular errors are already handled by the spawned child. Discarding the error here was a nice way to prevent the error from being printed twice:

As you can see, if I don't catch the error, I get the child info, which make little sense in the context of the workspace command (it should be an hidden implementation detail).

However, catching the error unfortunately also swallows the exit code ... what would you think about manually calling process.exit in the catch block, with the exit code returned by the child?

I think double error is ok-ish as long as it does not show the same error message twice.
I would vote against calling process.exit in multiple places in the code, better leave only one exit code in index.js, this makes code hard to reuse

… resolution

arcanis · 2017-05-18T10:17:21Z

I fixed the issue I raised above, rebased, and implemented the exec command. However, I'm not sure I'll be able to write the tests right now:

The current implementation forwards stdio to the spawn process (which is useful, because it allows the spawn processes to interact with the user, which would break otherwise). Unfortunately, it means that it bypasses the reporters, so I can't inspect to make sure that the output is the one I expect. I tried to workaround this by inspecting the state of the filesystem, but:
The current implementation uses process.argv[0] and process.argv[1] in order to spawn a new instance of Yarn. Unfortunately, doing this cannot work when running Jest, since it will run Jest instead of Yarn.

bestander · 2017-05-18T10:44:51Z

src/config.js

+
+    // We need the ignoreFiles, not the keepFiles, because the patterns are exclusion patterns rather than the opposite
+    const ignoreBasenames = new Set(this.registryFolders);
+    const files = filter.sortFilter(await fs.walk(root, undefined, ignoreBasenames), compiledPatterns).ignoreFiles;


Let's merge this code with the one used in install command https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/install.js#L238.

I think using glob is better than ad-hoc fs.walk because the JS community is quite familiar with glob notation.

The code above is also using the glob syntax, because of sortFilters. I tried to use the same implementation from what we use for pack, but maybe glob is more efficient, yeah.

Should be fine now 👍 Ok to merge?

bestander · 2017-05-19T13:28:13Z

src/config.js

+  }
+
+  async findWorktree(initial: string): Promise<?string> {
+    let previous = null;


install command is guarded by projectManifestJson.workspaces && this.config.workspacesExperimental.
How about extracting it in a setting and checking it here?

Ping about the question above.

I think it is related to my previous comment regarding config.worktreeFolder || config.cwd.
Should resolving workspaces and root folders be hidden behind the configuration flag and whether workspaces is enabled in the root manifest?

Not sure what you mean with projectManifestJson.workspaces - if we're not in a workspace (ie. if there is no manifest with a workspaces key), then findWorktree will return null. Do you want it to do something different?

I'll add a check on the experimental flag, tho.

I guess I got my thoughts mixed up a bit :)
I think resolveWorkspaces should check for experimental flag and private field.
Then this check would propagate to be implicit everywhere workspaces are used.

Hm I was thinking of implementing this check inside Config#init, would you be ok with that? The problem with resolveWorkspaces is that it doesn't have to be executed in every case (for example, when adding a dependency we don't use it, since we only care about the current workspace and the base worktree).

Yeah, definitely, you are correct

bestander · 2017-05-19T13:30:28Z

src/cli/commands/install.js

@@ -660,7 +660,7 @@ export class Install {
    }

    // build lockfile location
-    const loc = path.join(this.config.cwd, constants.LOCKFILE_FILENAME);
+    const loc = path.join(this.config.worktreeFolder || this.config.cwd, constants.LOCKFILE_FILENAME);


The this.config.worktreeFolder || this.config.cwd expression is used in multiple places.
How about moving it to a function that would be self explaining?
Should we make this.config.cwd private?

Hm, I think right now it's fine, but maybe that's something that could be improved later.

I was actually thinking that maybe we could add another variable, workspaceFolder, that would contain the path to the top-most workspace, and use it instead of cwd about everywhere, except of course yarn init.

The difference with cwd would be that if you're in a subdirectory, then the workspaceFolder would always target the directory that contains the package.json file. This way, people wouldn't have the issue of adding dependencies to their subfolders instead of their project, if they're in the wrong directory.

But that would be a BC, so we should probably discuss this in an other PR/RFC

The question is - someone adds a new command, say amazing-command, do they need to pass this.config.worktreeFolder || this.config.cwd when reading lockfile?

It looks like we leak workspaces implementation detail into every command, seems like a red flag and besides, we have config.worktreeFolder || repeated 13 times already. It definitely asks for encapsulation

Here are the options (I assume in these options that there is a workspaceFolder variable, but it work the same way if we don't):

Keep things as they are (eh), with the worktree definition being "the closest location where lies the workspaces field", and explicitely specify the locations from which we want to read the lockfile

Redefine worktreeFolder as being "the closest location where lies the lockfile", which would effectively worktreeFolder || workspaceFolder || cwd - the reason I didn't do this is that it means that a project with no workspaces would internally work just like a project with a single workspace (we wouldn't be able to distinguish a project that doesn't use workspaces like we currently can with worktreeFolder === null). That's not a problem strictly speaking, but we haven't specified this in our RFCs so we have to agree on this before I can do this.

Add a new variable (let's say lockfileFolder) that would be defined as worktreeFolder || workspaceFolder || cwd. The only issue I see with this is that it would mean yet another folder variable.

What do you think? My order of preference is 3 > 1 > 2

lockfileFolder sounds quite descriptive, I like it, let's try 3

bestander · 2017-05-19T13:30:49Z

Great progress, @arcanis.
We need more tests ;)

bestander · 2017-05-20T14:54:58Z

-- updated comment after thinking a bit --

I am working on phase 3, there is a need to preserve the workspaces structure during whole installation from fetch phase till linking.

In fetchRequestFromCwd at https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/install.js#L233 InstallCwdRequest is returned.
I wonder if this structure could be preserved through the whole installation and passed to linking.

sejoker · 2017-05-21T13:24:24Z

src/cli/commands/workspace.js

+
+  const workspaces = await config.resolveWorkspaces(worktreeFolder, manifest.workspaces);
+
+  if (args.length < 1) {


we could move args checks before await config.resolveWorkspaces(worktreeFolder, manifest.workspaces), will eliminate some extra work this way.

sejoker · 2017-05-21T13:33:56Z

src/lockfile/wrapper.js

    // read the manifest in this directory
-    const lockfileLoc = path.join(dir, constants.LOCKFILE_FILENAME);
+    const lockfileLoc = path.join(config.worktreeFolder || config.cwd, constants.LOCKFILE_FILENAME);


dir is not used anymore, what is the reason to keep it as a parameter?

Good catch! This code became boggus after I refactored it a few times. I've now restored the old code, since we don't use config in this function anymore 👍

bestander · 2017-05-22T15:11:10Z

__tests__/commands/workspace.js

+
+    expect(await fs.exists(`${config.cwd}/yarn.lock`)).toEqual(true);
+    expect(await fs.exists(`${config.cwd}/packages/package-b/yarn.lock`)).toEqual(false);
+  });


You also need to check in which package.json left-pad/right-pad entries were added.
And run yarn add from the root.

I think https://github.com/yarnpkg/yarn/blob/master/__tests__/commands/install/workspaces-install.js is a better location for this test as you are not testing workspace command here

bestander

Just a few small concerns about paths and config

bestander · 2017-05-22T15:21:44Z

src/config.js

+  }
+
+  async findWorktree(initial: string): Promise<?string> {
+    let previous = null;


Ping about the question above.

I think it is related to my previous comment regarding config.worktreeFolder || config.cwd.
Should resolving workspaces and root folders be hidden behind the configuration flag and whether workspaces is enabled in the root manifest?

arcanis · 2017-05-24T10:33:23Z

@bestander Everything should be fixed 👍

bestander

Awesome!
Merge once tests are green

bestander · 2017-07-22T02:24:48Z

Hey @arcanis, I think we lost this feature.
Running yarn workspace jest-matcher-utils add isarray does not run the add from workspace anymore.

jkillian · 2017-07-25T15:05:06Z

Ran into this same bug today, file a short ticket over at #4021

arcanis requested a review from bestander May 10, 2017 14:17

This was referenced May 10, 2017

Workspaces issue progress #3294

Closed

workspaces: first phase: install node_modules aggregated from all workspaces #3229

Merged

bestander reviewed May 11, 2017

View reviewed changes

Maël Nison added 2 commits May 15, 2017 13:51

Adds yarn workspace command

34cc999

Adds workspace support to install (/ add / remove / ...)

3683872

arcanis force-pushed the yarn-workspace branch from f628dfb to 3683872 Compare May 15, 2017 15:50

arcanis mentioned this pull request May 16, 2017

Workspaces phase 2: commands to manage workspaces yarnpkg/rfcs#61

Merged

bestander reviewed May 16, 2017

View reviewed changes

Maël Nison and others added 4 commits May 17, 2017 15:59

Avoids multiple invocations of "findWorktree"

8e266c3

Adds yarn exec

f80218e

Merge branch 'master' into yarn-workspace

a736e9f

Prevents the node_modules folder from being read during the workspace…

c35faaf

… resolution

Fixes flow

fd6bcb9

bestander reviewed May 18, 2017

View reviewed changes

Maël Nison added 3 commits May 18, 2017 14:18

Fixes lint

287bdfc

Reuses resolveWorkspaces

beb2c82

Fixes linting

6876064

bestander reviewed May 19, 2017

View reviewed changes

bestander changed the title ~~Adds yarn workspace command~~ Workspaces phase 2: executing commands May 19, 2017

sejoker reviewed May 21, 2017

View reviewed changes

Maël Nison added 4 commits May 22, 2017 13:01

Refactors things

698fb08

Moves args validation before we do more work

097644f

Fixes linting

18e8ed9

Fixes workspace resolution bug

cafedd6

bestander reviewed May 22, 2017

View reviewed changes

Maël Nison and others added 4 commits May 23, 2017 10:58

Moves the experimental check into the config init

bffc14d

Adds lockfileFolder

e7a1282

Moves the test into add

8ea605b

Merge branch 'master' into yarn-workspace

dd994f3

bestander approved these changes May 24, 2017

View reviewed changes

Maël Nison added 3 commits May 24, 2017 13:30

Fixes linting

f567fcf

Fixes hoister tests

ddcba1d

Fixes tests

a15b821

arcanis force-pushed the yarn-workspace branch from aba62fb to a15b821 Compare May 24, 2017 14:48

arcanis merged commit 8ede3c4 into yarnpkg:master May 24, 2017

ethanherbertson mentioned this pull request Feb 4, 2020

yarn help exec points to wrong docs link #7172

Closed


		const workspaces = await config.resolveWorkspaces(worktreeFolder, manifest.workspaces);

		if (args.length < 1) {

Workspaces phase 2: executing commands #3365

Workspaces phase 2: executing commands #3365

Conversation

arcanis commented May 10, 2017

arcanis commented May 10, 2017

bestander commented May 10, 2017

bestander commented May 10, 2017

arcanis commented May 10, 2017

bestander commented May 10, 2017

bestander commented May 10, 2017

arcanis commented May 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis commented May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bestander left a 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

Choose a reason for hiding this comment

arcanis May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis commented May 18, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bestander commented May 19, 2017

bestander commented May 20, 2017 • 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

bestander left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis commented May 24, 2017

bestander left a comment

Choose a reason for hiding this comment

bestander commented Jul 22, 2017

jkillian commented Jul 25, 2017

arcanis commented May 15, 2017 •

edited

Loading

arcanis May 17, 2017 •

edited

Loading

arcanis May 22, 2017 •

edited

Loading

arcanis May 22, 2017 •

edited

Loading

arcanis May 22, 2017 •

edited

Loading

bestander commented May 20, 2017 •

edited

Loading