Workspaces phase 3: linking #66

bestander · 2017-05-18T16:34:41Z

Linking between workspaces

bestander · 2017-05-18T16:41:50Z

Asking for comments from @arcanis @thejameskyle @cpojer @zertosh @Daniel15

sejoker · 2017-05-21T14:39:58Z

accepted/0000-workspaces-link-phase-3.md

+  "dependencies": {
+    "chalk": "^1.1.3",
+    "diff": "^3.2.0",
+    "**jest-matcher-utils**": "^20.0.3",


doesn't render as bold.

Thanks, apparently I can't mix markup into code blocks

jamiebuilds · 2017-05-22T00:04:23Z

This is a bit separate, but one of the important things to keep in mind about the linking phase is what order you run the postinstall/prepublish scripts. It's possible for one workspace to depend on another another workspace within the same project and to use it as part of these scripts.

If you run the scripts in the wrong order on the workspaces, one might not be ready by the time another one goes to use it.

Lerna sorts them topologically based on which workspaces depend on one another. But that still has problems with circular dependencies.

I've been thinking about ways to mitigate this.

Give different priorities to dependency sorting so that "devDependencies" are treated more important for build scripts than "dependencies"
Add a way of resolving circular dependencies either as an additional field in workspace's package.json or in the project's package.json
Allow the Project's package.json#workspaces field specify dependencies in the order they should be built in:

{
  "workspaces": [
    "packages/build-tool",
    "packages/*"
  ]
}

We could also combine some of these, they could all work together.

But the sorting process should definitely alert the user if it is unable to sort the workspaces definitively.

bestander · 2017-05-22T16:38:22Z

Good points, @thejameskyle.
I'll add this to the RFC

cpojer · 2017-05-24T10:42:22Z

accepted/0000-workspaces-link-phase-3.md

+
+In particular, testing packages that refer other packages from the same codebase can be difficult because Node.js and front end bundling tools would look up the referred packages in node_modules folder as it should be installed from npm registry.
+
+Yarn Workspaces need to enable referring other local packages the same way when local packages are in development mode (source of truth is the package source code) and in production mode (source of truth is the package installed from npm).


nit: "need to be able to refer to other local packages"

cpojer · 2017-05-24T10:42:31Z

accepted/0000-workspaces-link-phase-3.md

+| ---- package.json
+| ---- packages/
+| -------- jest-matcher-utils/
+| ------------ packjage.json


package.json here and below.

cpojer · 2017-05-24T10:43:38Z

accepted/0000-workspaces-link-phase-3.md

+```
+{
+  "name": "jest-matcher-utils",
+  "description": "A set of utility functions for jest-matchers and related packages",


Can you get rid of all the non-essential stuff from this package.json and replace it with …?

cpojer · 2017-05-24T10:43:59Z

accepted/0000-workspaces-link-phase-3.md

+| ---------------- **jest-matcher-utils**/  (symlink) -> ../jest-matcher-utils
+| ------------ package.json
+...
+```


nice description of the solution.

cpojer · 2017-05-24T10:44:57Z

accepted/0000-workspaces-link-phase-3.md

+
+### Dependencies and version matching
+
+Yarn would only link workspaces to each other if they match semver conditions.


As @arcanis pointed out, we should absolutely warn in the case that it is not. In Jest, for example, this has caused issues in the past and we didn't know about it. Can you add it to the RFC?

I don't think it should always warn. This breaks Babel's use case where it is self-hoisting and wants to use an older version intentionally.

I suppose it could report which packages are linked after install.
I agree with @thejameskyle that it can be annoying if it is intended

But I think it is early to spec this detail, we'll need to see what level of reporting works here.
Definitely it should be in verbose logs

cpojer · 2017-05-24T10:45:36Z

accepted/0000-workspaces-link-phase-3.md

+| ------------ package.json
+| -------- jest-diff/
+| ------------ node_modules/
+| ---------------- **jest-matcher-utils**/  (symlink) -> ../jest-matcher-utils


Can you write down in words that the symlink is relative, just to make sure we have this captured properly?

cpojer · 2017-05-24T10:46:08Z

accepted/0000-workspaces-link-phase-3.md

+
+As long as **jest-matcher-utils** does not make relative requires via its parent folder, flag **--preserve-symlinks** won't be necessary.
+
+


rm empty duplicate line.

cpojer · 2017-05-24T10:46:49Z

accepted/0000-workspaces-link-phase-3.md

+
+This solution creates a symlink inside node_modules of a Workspace package and symlinks have multiple drawbacks:
+
+* Symlinks are not supported in all tools (e.g. watchman)


We can potentially punt on this for a while.

cpojer · 2017-05-24T10:47:40Z

accepted/0000-workspaces-link-phase-3.md

+This solution creates a symlink inside node_modules of a Workspace package and symlinks have multiple drawbacks:
+
+* Symlinks are not supported in all tools (e.g. watchman)
+* Symlinks are not supported well in all OS and environments (Windows pre 10 Creative updated, Docker on SMB storage(?))


Could we have an alternative mode, especially for CI, where we run "yarn build" and then "yarn pack" on every package and cp the build into node_modules instead of using symlinks? CI's don't need the symlinks because you aren't changing the code there. Symlinks are only really important during development.

Running yarn build implies that the module has its dependencies ready, which might not be the case when working with cyclic dependencies.

I've seen once a cyclic dependency with post install scripts.
Not a common but possible case.
What if we just don't support cyclic workspaces?
It seems like a poor design and causes a lot of trouble.

cpojer · 2017-05-24T10:51:33Z

accepted/0000-workspaces-link-phase-3.md

+
+* Symlinks are not supported in all tools (e.g. watchman)
+* Symlinks are not supported well in all OS and environments (Windows pre 10 Creative updated, Docker on SMB storage(?))
+* A symlink to **jest-match-utils** does not emulate actual installation of the package, it just symlinks to the package source code - no prepublish and postinstall lifecycle scripts are executed and no files are filtered (as done during publishing)


You are using the wrong module name ;)

I think it is correct.
jest-diff -> jest-match-utils

It's jest-matcher-utils.

cpojer · 2017-05-24T10:52:20Z

accepted/0000-workspaces-link-phase-3.md

+
+## Unresolved questions
+
+* Is there still an issue with Node resolving real paths in symlinked folders (https://github.com/nodejs/node/issues/3402)? I think if all node_modules are installed in the Workspace root which is a parent to all workspaces and there are no relative (via ../../...) requires between workspaces everything should be working fine.


I don't think RFCs should have language like "I think" :D Mind making this more objective?

cpojer · 2017-05-24T10:52:57Z

accepted/0000-workspaces-link-phase-3.md

+
+* Is there still an issue with Node resolving real paths in symlinked folders (https://github.com/nodejs/node/issues/3402)? I think if all node_modules are installed in the Workspace root which is a parent to all workspaces and there are no relative (via ../../...) requires between workspaces everything should be working fine.
+* Should the symlinks be created in Workspace root node_modules instead of referring Workspaces?
+* Any special treatment for scoped packages?


Can you answer this? It seems easy to solve, they just need to be supported normally, no?

I don't think there's any need to treat them specially. Maybe I'm missing something though

Ok, just wanted to confirm it in first iteration

cpojer · 2017-05-24T10:58:20Z

accepted/0000-workspaces-link-phase-3.md

+* Should the symlinks be created in Workspace root node_modules instead of referring Workspaces?
+* Any special treatment for scoped packages?
+* Does it need to work for other type of packages: git, file, etc?
+* As described in Workspace phase 1 RFC (https://github.com/yarnpkg/rfcs/pull/60) there is only one lockfile per workspace. Should we have workspaces that are referred from other workspaces referred in the root lockfile?


This is a good question. Do we only want to enable one gigantic workspace or should we enable slices of projects that compose different workspaces (this may not be possible)? For example, if you take Jest and react-native-packager, they have some overlapping modules: would you have two workspaces, one for each project where some modules are shared or would you have one workspace for both projects?

The reason I'm asking is because at FB, if we end up linking 100 packages together, that may actually be slower in the end and managing them outside a workspace may be better. Not something that blocks the current RFC and feature, but something to keep in mind.

This is a different question, I'll rephrase it.
I think the one that you raised is out of scope for this RFC and we'll need to get back to it later.

cpojer

Nice work. I requested a few clarifications here and there and a bunch of nits.

One of the larger issues I'm worried about is that we aren't discussing about build steps here. If we were to create a workspace that contains projects from two separate parent projects that have a different build pipeline, for example one of them uses TypeScript and the other one uses babel, then we don't know how to build the project. This doesn't need to be addressed now, but I believe this needs to be on our mind long term. Imagine Jest's watch mode, instead of running tests, watching for file changes and building different projects whenever something happens ;P

cpojer · 2017-05-24T11:01:47Z

Also, can you elaborate on the decision to deprecate yarn knit or do it in a separate PR? I don't think it fits into this PR.

bestander · 2017-05-25T11:47:24Z

Also, can you elaborate on the decision to deprecate yarn knit or do it in a separate PR? I don't think it fits into this PR.

I am rolling this back.
I thought that yarn knit would be deprecated by workspaces and linking but probably we would want to have both anyway and some code could be shared.

bestander · 2017-05-25T12:25:51Z

One of the larger issues I'm worried about is that we aren't discussing about build steps here. If we were to create a workspace that contains projects from two separate parent projects that have a different build pipeline, for example one of them uses TypeScript and the other one uses babel, then we don't know how to build the project. This doesn't need to be addressed now, but I believe this needs to be on our mind long term. Imagine Jest's watch mode, instead of running tests, watching for file changes and building different projects whenever something happens ;P

There are 2 aspects that I missed:

build order of install scripts (addressed by @thejameskyle https://gist.github.com/thejameskyle/abbc146a8cb5c26194c8acc4d14e7c30)
Sharing workspaces between multiple roots.
I would keep it out of scope for now.

cpojer

I think this looks solid from my perspective. There is a lot more we'll probably have to do on top of this RFC, but it is a good start and will solve the basic problem for most cases.

bestander added 4 commits May 18, 2017 14:58

moved implemented and deprecated rfcs to the right folders

2a3801d

workspaces phase 3 RFC

abc4a18

wording

4e0a0db

wording

6552910

bestander mentioned this pull request May 18, 2017

Workspaces issue progress yarnpkg/yarn#3294

Closed

22 tasks

bestander changed the title ~~Workspaces phase 3~~ Workspaces phase 3: linking May 18, 2017

added a note to alternatives

f639d2b

sejoker reviewed May 21, 2017

View reviewed changes

cpojer reviewed May 24, 2017

View reviewed changes

cpojer suggested changes May 24, 2017

View reviewed changes

bestander added 2 commits May 25, 2017 13:27

addressed comments and undeprecated knit

8147142

added a section about install scripts order and cycles

98a5686

added note about forgetting to add dependencies to workspaces

5f49f89

cpojer approved these changes May 25, 2017

View reviewed changes

added a note

a6a81c9

bestander merged commit c1112fb into yarnpkg:master May 30, 2017

bestander mentioned this pull request May 30, 2017

Workspace phase 3 & 4 yarnpkg/yarn#3516

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workspaces phase 3: linking #66

Workspaces phase 3: linking #66

bestander commented May 18, 2017 •

edited

Loading

bestander commented May 18, 2017

sejoker May 21, 2017

bestander May 22, 2017

jamiebuilds commented May 22, 2017

bestander commented May 22, 2017

cpojer May 24, 2017

cpojer May 24, 2017

cpojer May 24, 2017

cpojer May 24, 2017

cpojer May 24, 2017

jamiebuilds May 24, 2017

bestander May 25, 2017

bestander May 25, 2017

cpojer May 24, 2017

cpojer May 24, 2017

cpojer May 24, 2017

cpojer May 24, 2017

arcanis May 24, 2017

bestander May 25, 2017

cpojer May 24, 2017

bestander May 25, 2017

cpojer May 25, 2017

bestander May 25, 2017

cpojer May 24, 2017

cpojer May 24, 2017

jamiebuilds May 24, 2017

bestander May 25, 2017

cpojer May 24, 2017

bestander May 25, 2017

cpojer left a comment

cpojer commented May 24, 2017

bestander commented May 25, 2017

bestander commented May 25, 2017

cpojer left a comment


		In particular, testing packages that refer other packages from the same codebase can be difficult because Node.js and front end bundling tools would look up the referred packages in node_modules folder as it should be installed from npm registry.

		Yarn Workspaces need to enable referring other local packages the same way when local packages are in development mode (source of truth is the package source code) and in production mode (source of truth is the package installed from npm).


		### Dependencies and version matching

		Yarn would only link workspaces to each other if they match semver conditions.


		As long as jest-matcher-utils does not make relative requires via its parent folder, flag --preserve-symlinks won't be necessary.


		This solution creates a symlink inside node_modules of a Workspace package and symlinks have multiple drawbacks:

		* Symlinks are not supported in all tools (e.g. watchman)


		## Unresolved questions

		* Is there still an issue with Node resolving real paths in symlinked folders (https://github.com/nodejs/node/issues/3402)? I think if all node_modules are installed in the Workspace root which is a parent to all workspaces and there are no relative (via ../../...) requires between workspaces everything should be working fine.

Workspaces phase 3: linking #66

Workspaces phase 3: linking #66

Conversation

bestander commented May 18, 2017 • edited Loading

bestander commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamiebuilds commented May 22, 2017

bestander commented May 22, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpojer left a comment

Choose a reason for hiding this comment

cpojer commented May 24, 2017

bestander commented May 25, 2017

bestander commented May 25, 2017

cpojer left a comment

Choose a reason for hiding this comment

bestander commented May 18, 2017 •

edited

Loading