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.

Sign up for GitHub

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

Jump to bottom

Workspace phase 3 & 4 #3516

Merged
merged 4 commits into from
Jun 2, 2017
Merged

Workspace phase 3 & 4 #3516

merged 4 commits into from
Jun 2, 2017

Conversation

bestander
Copy link
Member

@bestander bestander commented May 27, 2017 •
edited
Loading

Implements Workspaces phase 3 & 4 RFC - ability to link between workspaces.

Description

The structure of the source code is following

| jest/
| ---- package.json
| ---- packages/
| -------- jest-matcher-utils/
| ------------ package.json
| -------- jest-diff/
| ------------ package.json
...

Top level package.json is like

{
  "private": true,
  "name": "jest",
  "devDependencies": {
  },
  "workspaces": [
    "packages/*"
  ]
}

jest-matcher-utils (workspace referred by another one)

{
  "name": "jest-matcher-utils",
  "description": "...",
  "version": "20.0.3",
  "repository": {
    "type": "git",
    "url": "https://github.com/facebook/jest.git"
  },
  "license": "...",
  "main": "...",
  "browser": "...",
  "dependencies": {
    "chalk": "^1.1.3",
    "pretty-format": "^20.0.3"
  }
}

jest-diff (workspace that refers jest-matcher-utils)

{
  "name": "jest-diff",
  "version": "20.0.3",
  "repository": {
    "type": "git",
    "url": "https://github.com/facebook/jest.git"
  },
  "license": "...",
  "main": "...",
  "browser": "...",
  "dependencies": {
    "chalk": "^1.1.3",
    "diff": "^3.2.0",
    "**jest-matcher-utils**": "^20.0.3",
    "pretty-format": "^20.0.3"
  }
}

When user runs yarn install, this folder structure of the Workspace gets created

| jest/
| ---- node_modules/
| -------- chalk/
| -------- diff/
| -------- pretty-format/
| ---- package.json
| ---- packages/
| -------- jest-matcher-utils/
| ------------ node_modules/ (empty, all dependencies hoisted to the root)
| ------------ package.json
| -------- jest-diff/
| ------------ node_modules/
| ---------------- **jest-matcher-utils**/  (symlink) -> ../jest-matcher-utils
| ------------ package.json
...

jest/packages/jest-diff/node_modules/**jest-matcher-utils** is a relative symlink to jest/packages/jest-matcher-utils

Test Plan

  • Added tests

@bestander bestander mentioned this pull request May 27, 2017
Closed
22 tasks
@bestander bestander changed the title [WIP] Workspace phase 3 [WIP] Workspace phase 3 & 4 May 30, 2017
@bestander bestander force-pushed the workspace-phase-3 branch 8 times, most recently from 5aa907a to 61d181b Compare June 1, 2017 10:43
@bestander bestander force-pushed the workspace-phase-3 branch from 61d181b to 4377211 Compare June 1, 2017 11:22
@bestander bestander changed the title [WIP] Workspace phase 3 & 4 Workspace phase 3 & 4 Jun 1, 2017
@bestander
Copy link
Member Author

In a follow up PR I would look into testing check command more and also removing duplication of patterns array cleanup (I remove workspaces from it in a few places).

@bestander bestander requested a review from arcanis June 1, 2017 18:55
arcanis
arcanis reviewed Jun 2, 2017
View reviewed changes
src/cli/commands/install.js
!this.flags.ignoreOptional,
);

if (this.config.workspacesEnabled) {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this check is necessary - if there's a workspace but the experimental option is missing, yarn will abort anyway (https://github.com/yarnpkg/yarn/blob/master/src/config.js#L303)

Copy link
Member Author

Choose a reason for hiding this comment

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

indeed, I'll remove the if

Copy link
Member Author

Choose a reason for hiding this comment

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

Turns out this condition is still needed for commands that need fetchRequestFromCwd but have workspaces disabled.
It would try to add workspaces resolution anyway.

src/cli/commands/install.js Outdated
workspaces[virtualDependencyManifest.name] = {loc: '', manifest: virtualDependencyManifest};
virtualDependencyManifest.dependencies = {};
for (const workspaceName of Object.keys(workspaces)) {
virtualDependencyManifest.dependencies[workspaceName] = workspaces[workspaceName].manifest.version;
Copy link
Member

Choose a reason for hiding this comment

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

This line will make the virtual dependency manifest depend on itself

Copy link
Member Author

Choose a reason for hiding this comment

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

good find!

src/config.js Outdated

//
cwd: string;
worktreeFolder: ?string;
workspaceFolder: ?string;
Copy link
Member

Choose a reason for hiding this comment

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

There's an ambiguity with this name, since it's not clear if it's the path to the workspace common root or the path to a specific workspace.

Copy link
Member Author

Choose a reason for hiding this comment

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

how about rootFolder?

Copy link
Member

@arcanis arcanis Jun 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'd prefer workspaceRootFolder so that the implication that the variable is null when not using workspaces is made clear. But why not keep worktree as the designated term for the workspace roots? It clear and concise! 😛

Copy link
Member Author

Choose a reason for hiding this comment

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

I just wanted to not introduce a new term worktree because it does not imply that it is only relevant in workspaces.
workspaceRootFolder sounds quite clear.

@bestander
Copy link
Member Author

Thanks, @arcanis.
I'll merge once CI is green to avoid merge conflicts

@bestander bestander merged commit 4463175 into yarnpkg:master Jun 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arcanis arcanis arcanis approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bestander @arcanis