Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Workspaces phase 2: commands to manage workspaces #61

Merged
merged 3 commits into from
May 17, 2017

Conversation

arcanis
Copy link
Member

@arcanis arcanis commented May 5, 2017

No description provided.

@arcanis arcanis requested review from cpojer and bestander May 5, 2017 15:17

# Alternatives

- Is `yarn project` a good name? I considered `yarn workspace`, but we're not operating on the workspace itself, just its projects. I feel like a `yarn workspace` command should only affect the root package.json (the one who contains the `workspaces` directive).
Copy link
Member

@bestander bestander May 5, 2017

Choose a reason for hiding this comment

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

We planned to use workspaces keyword in package.json to identify all sub projects.
I would also try to stay away from term project because this term is overused in IT.
But then what term should we use for the root folder?

Copy link

Choose a reason for hiding this comment

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

As if not overused already: container

Copy link
Member

Choose a reason for hiding this comment

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

I think we should limit the number of terms if possible.
I propose workspaces root and workspaces

Copy link
Member Author

@arcanis arcanis May 16, 2017

Choose a reason for hiding this comment

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

@thejameskyle suggested "project" for the workspaces root, and "workspace" for the ... workspaces.

I also had another idea: make the root "workbase". Keep in mind that I'm not a native english speaker so maybe it doesn't make sense, but it feels like a nice way to clearly show that a workspace is contained inside a workbase (because of the "base" suffix). Without this, I fear people will be confused. If "base" doesn't work, maybe something else? Worktree, workroot, workset, ...

@jamiebuilds
Copy link
Contributor

See yarn exec and yarn workspaces exec / yarn ws exec in: #62


# Motivation

With the addition of workspaces, it will become handy to be able to execute commands inside other projects than the one in the current directory.
Copy link
Member

Choose a reason for hiding this comment

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

There are some specific scenarios that we need to address in this RFC.

For example, from my RFC #60

The structure of the source code is following

| jest/
| ---- package.json
| ---- packages/
| -------- babel-jest/
| ------------ packjage.json
| -------- babel-preset-jest/
| ------------ packjage.json
...

We want to address how we run commands (install/add/upgrade/remove) when CWD is workspace root.
Then what happens if CWD is jest/packages/babel-preset-jest?

How do I update a dependency in a workspace and in a root?

Copy link
Member Author

Choose a reason for hiding this comment

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

You can use yarn add to add a dependency in the current workspace, or yarn workspace my-workspace add to add a dependency inside a specific workspace from anywhere in the project tree.

@arcanis
Copy link
Member Author

arcanis commented May 16, 2017

Matching implementation: yarnpkg/yarn#3365. Still a work-in-progress, but it seems to work relatively well. I'll clean it this afternoon, but still happy to hear feedbacks!

@thejameskyle I think the command described in this RFC is very similar to the one you described in your own RFC. Do you want to discuss here this specific part of the Workspace feature? I think we can reasonably also talk about your idea of yarn workspace exec, which share a similar scope 🙂


```
$> yarn workspace <project-name> pwd
$> yarn workspace <project-name> <command-name> ...
Copy link
Contributor

@jamiebuilds jamiebuilds May 16, 2017

Choose a reason for hiding this comment

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

One of the things I'd get a bit worries about here is overloading the same command space. There's three different things that fit into this space broken down into two groups:

  • yarn workspace <name> <command/run-script> which matches yarn <command/run-script>
  • yarn workspace <name> <system-command> (i.e. yarn workspace <name> pwd)

That's why I would like to see a new global command exec (which is similar to lerna exec)

project-level workspace-level
yarn <command/run-script> yarn workspace <name> <command/run-script>
yarn run <run-script> yarn workspace <name> run <run-script>
yarn exec <system-command> yarn workspace <name> exec <system-command>

Copy link
Member Author

@arcanis arcanis May 16, 2017

Choose a reason for hiding this comment

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

Hm, I see what you mean. The pwd command could be delegated to shell via exec. However, I think it would break the Windows portability. Since the command would run in cmd.exe, the pwd utility wouldn't be available, and users would have to use cd instead. In the end, the command would look like:

$> yarn workspace babel-core exec pwd || yarn workspace babel-core exec cd

Which looks awfully verbose (and not great perf-wise). Adding a builtin pwd command would solve this at virtually no cost, since we would need to compute the path anyway.

Regarding the exec command, I'm not sure how people will end up using it (they should probably register their commands as scripts if they intend to use them regularly, or to use advanced shell structures), but I have no objection implementing it, the maintenance cost shouldn't be very high anyway.

Just one concern: since Yarn already has a run command, aren't you worried that users might be confused regarding the command to use, depending on whether they want to run a script or a shell command?

Copy link
Contributor

Choose a reason for hiding this comment

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

We've been using lerna run and lerna exec for a little while. I haven't heard of anyone confused by it. I think we can mitigate that with documentation if it does become an issue.

npm/Yarn run commands are already open to cross-platform incompatibilities since you could put pwd in package.json#scripts and have the same cross-platform problems. People (including me) still think its useful regardless.

Copy link
Contributor

@jamiebuilds jamiebuilds May 16, 2017

Choose a reason for hiding this comment

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

The other thing that we can do to make this useful cross-platform is to specify the $PATH the way package.json#scripts do.

From #62:

There's three options for what the $PATH should be within workspaces.

  • The project's node_modules/.bin
  • The workspace's node_modules/.bin
  • Both (w/ workspace before project)

This is useful because people can add packages with CLIs that do things cross platform.

Copy link
Member Author

Choose a reason for hiding this comment

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

We've been using lerna run and lerna exec for a little while. I haven't heard of anyone confused by it. I think we can mitigate that with documentation if it does become an issue.

Fine by me, then.

The other thing that we can do to make this useful cross-platform is to specify the $PATH the way package.json#scripts do.

Ooooh, I like it, I often end up writing "env": "env" in my scripts just because I want to see the env my scripts will use. That could be an interesting use case for yarn exec (no workspace).

Ok, I'll update the document to add this.


# Drawbacks

- We will still have the issue of requiring the `--` separator to forward any command line option (`yarn workspace test install -- --production`). It's an important issue, something we really should tackle sooner than later.
Copy link

Choose a reason for hiding this comment

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

I think we need to work through this. Yarn's own first-class flags should never require -- to be passed. If you run yarn workspace test jest -- --verbose it makes sense, but not if it is part of a Yarn command.

Copy link

Choose a reason for hiding this comment

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

I wouldn't block this RFC on it though, I'm just saying I agree we need to make this work.

Copy link
Member

@bestander bestander left a comment

Choose a reason for hiding this comment

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

Great work, guys.
Good to be merged, @thejameskyle, @cpojer?

@jamiebuilds
Copy link
Contributor

Do we want to limit it to valid set of sub-commands that Yarn will run inside workspaces?

@arcanis
Copy link
Member Author

arcanis commented May 17, 2017

Implementation-wise I don't think it's necessary, and I'd prefer to avoid maintaining a whitelist. Some commands will make little sense in a workspace context, sure, but who knows, maybe people will come up with some creative uses.

Ok! I'll merge this this afternoon if everyone's ok with this, and finish the PR in the train 👍

@arcanis arcanis merged commit ec136a6 into yarnpkg:master May 17, 2017
@arcanis arcanis deleted the workspace-command branch May 17, 2017 12:51
@bestander bestander changed the title Adds workspace command Workspaces phase 2: commands to manage workspaces May 18, 2017
@jardakotesovec
Copy link

Its not clear to me if is possible to install only subset of workspaces. yarn install in root folder install all workspaces. But in some cases (deployment of individual workspaces) we would like to install one or few of the workspaces, but still that it would install to the node_modules in top folder and would be merged with top package.json.

Could be such use case considered?

@arcanis
Copy link
Member Author

arcanis commented Jun 2, 2017

I think we eventually will, but right now installing a worktree will install the dependencies for all the workspaces at once.

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.

5 participants