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: first phase: install node_modules aggregated from all workspaces #3229

Merged
merged 4 commits into from May 11, 2017

Conversation

bestander
Copy link
Member

@bestander bestander commented Apr 22, 2017

Workspaces phase 1: aggregated node_modules according to RFC yarnpkg/rfcs#60.

This change aggregates dependencies from sub projects

For example, if the structure of the source code is following

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

Top level package.json is like

{
  "private": true,
  "name": "jest",
  "devDependencies": {
    "ansi-regex": "^2.0.0"
  },
  "workspaces": [
    "packages/*"
  ]
}

babel-jest

{
  "name": "babel-jest",
  "description": "Jest plugin to use babel for transformation.",
  "version": "19.0.0",
  "repository": {
    "type": "git",
    "url": "https://github.com/facebook/jest.git"
  },
  "license": "BSD-3-Clause",
  "main": "build/index.js",
  "dependencies": {
    "babel-core": "^6.0.0",
    "babel-plugin-istanbul": "^4.0.0",
    "babel-preset-jest": "^19.0.0"
  }
}

babel-preset-jest

{
  "name": "babel-preset-jest",
  "version": "19.0.0",
  "repository": {
    "type": "git",
    "url": "https://github.com/facebook/jest.git"
  },
  "license": "BSD-3-Clause",
  "main": "index.js",
  "dependencies": {
    "babel-plugin-jest-hoist": "^19.0.0"
  }
}

If workspaces is enabled and yarn install is run at the root level of jest Yarn would install dependencies as if the package.json contained all the dependencies of all the package.json files combined, i.e.

{
  "devDependencies": {
    "ansi-regex": "^2.0.0",
  },
  "dependencies": {
    "babel-core": "^6.0.0",
    "babel-plugin-istanbul": "^4.0.0",
    "babel-preset-jest": "^19.0.0",
    "babel-plugin-jest-hoist": "^19.0.0"
  }
}

A single yarn.lock will be created at the root level and no node_modules and yarn.lock files will be generated for any of the workspaces.

Note: If there are different versions of the same package between workspaces or a workspace and the root then install will fail with error Dependency $0 has different versions in $1 and $2.
In a next phase conflicts will be resolved similar to package-hoister that works when node_modules folder is being built.

To set up.

Set project's private filed to true (so that people would not accidentally publish projects with non standard workspaces field)

{
  "private": true
}

Run

yarn config set workspaces-experimental true

if (root[type] && root[type][key] && root[type][key] !== workspaceJson[type][key]) {
this.reporter.warn(
this.reporter.lang('incompatibleDependenciesInWorkspace', key, workspaceCwd, rootCwd),
);
Copy link
Member

Choose a reason for hiding this comment

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

Maybe two extra modes could be interesting:

  • Merge dependency requirements (so that the range would become ~1.0.0 ~1.0.3, for example)
  • When conflict arise, prevent hoisting the conflicting modules (so that they end up being installed into their respective node_modules folders)

Copy link
Member

Choose a reason for hiding this comment

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

When conflict arise, prevent hoisting the conflicting modules (so that they end up being installed into their respective node_modules folders)

Problem being that in such a case, we would have to run yarn install recursively into the workspace directories

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we'll do it in a later phase.

@bestander bestander changed the title Don't merge: workspaces proof of concept workspaces: first phase: install node_modules aggregated from all workspaces May 9, 2017
@bestander
Copy link
Member Author

ping @arcanis for tests

@bestander
Copy link
Member Author

This PR will be merged at the same time with #3365 and then we'll ship the feature to for testing behind the flag.

Next step would be package hoisting logic at the workspaces level similar to node_modules hoisting Yarn does.
That would require a new structure in project manifest to be passed through to the linking phase.

Copy link
Member

@arcanis arcanis left a comment

Choose a reason for hiding this comment

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

Shouldn't this code also check if the cwd is inside a project and, if it is, run the install from the project root instead of the workspace (or at least prevent the install)? Otherwise, if people run yarn install from one of their workspace, they will create a node_modules that will shortcut the one created in the project root.

throw new MessageError(this.reporter.lang('workspacesRequirePrivateProjects'));
}
for (const workspace of projectManifestJson.workspaces) {
for (const workspaceLoc of await fs.glob(path.join(rootCwd, workspace, filename))) {
Copy link
Member

Choose a reason for hiding this comment

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

I've added a few utility methods to the Config class to support workspaces (for example https://github.com/yarnpkg/yarn/pull/3365/files#diff-9d9a6cd82f41984872a66a3ab0d440c4R555), it would make sense to update the code to use them once we've merged this PR, what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

does it support glob?
I think we can discuss that in the following PR

@bestander
Copy link
Member Author

Shouldn't this code also check if the cwd is inside a project and, if it is, run the install from the project root instead of the workspace (or at least prevent the install)?

Yeah, it makes sense.
Let's follow up with a check for this, I think it is tightly related to your PR anyway

@arcanis
Copy link
Member

arcanis commented May 11, 2017

👍 Good to merge, then?

@bestander bestander merged commit 20e5779 into yarnpkg:master May 11, 2017
@bestander
Copy link
Member Author

Thanks

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.

None yet

2 participants