Integrated with Yarn workspaces #3906

bestander · 2017-06-25T19:20:26Z

Summary

Yarn 0.27 has workspaces feature that should be a better alternative to Lerna boostrapping.
Yarn workspaces still don't replace Lerna (and probably won't do it completely for a long time), so Lerna publishing is still used for Jest.

This PR replaces Lerna bootstrap phase with Yarn workspaces.

Things to discuss:

Unlike Lerna Yarn hoists workspaces to the root node_modules.

It means that integration/examples test don't run in isolation from the workspace root so I removed symlinking to babel-jest et al. at the tests boostrapping.
One test integration_tests/__tests__/transform.test.js required to be running in isolation, so I changed the test bootstrap to execute on a copy on a temp folder.

As of Yarn 0.27.2 workspaces are still fresh, I am working on making it snappy.
Known issues:

changes in workspaces (non root) package.json are not automatically invalidating integrity check
logs may have confusing lines when working with workspaces
yarn add/upgrade/remove is not fully tested from within workspaces

PR to support Yarn workspaces as first class citizen in Lerna Support Yarn workspaces to replace bootstrap command lerna/lerna#899

Test plan

yarn test-ci
lint/flow/build tasks work per workspaces correctly
ran yarn run publish and lerna seems to handle version bumping and npm publishing correctly

bestander · 2017-06-25T19:27:33Z

We need to bump Yarn 0.27 to stable to fix CI.

cpojer · 2017-06-27T08:57:07Z

This is super sweet @bestander!

Mind rebasing and giving me an update on what the remaining problems are on this diff?

bestander · 2017-06-27T17:49:23Z

@cpojer will do.
What do you think of integration_tests/__tests__/transform.test.js change?
Yarn hoists all packages as symlinks to the top so all tests now have babel-jest available by default and we can't test integration/example tests in isolation anymore.

thymikee · 2017-06-27T17:52:22Z

If I recall correctly we're already using os. tmpdir in some of integration tests so I'm not opposed to do it in this case.

cpojer · 2017-06-27T18:22:07Z

Yep, I'm fine with it also.

bestander · 2017-06-29T00:24:38Z

Waiting for Yarn 0.27.2 release to be stable so that CI would run installation correctly

thymikee · 2017-07-04T17:47:24Z

@bestander is yarn stable now?

bestander · 2017-07-04T18:12:22Z

Yes

…

On Tue, Jul 4, 2017 at 10:47 AM Michał Pierzchała ***@***.***> wrote: @bestander <https://github.com/bestander> is yarn stable now? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3906 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWMiDz9h7tLKElwPrEsWAls1iychmks5sKnqwgaJpZM4OEsZ-> .

thymikee · 2017-07-04T18:22:16Z

Would you mind rebasing this diff when you have a while? :)

bestander · 2017-07-04T18:32:29Z

Will do today. Do you have any background on the es5 tests that are failing? It seems like hoisting of workspace folders to the root affects the bundle

…

On Tue, Jul 4, 2017 at 11:22 AM Michał Pierzchała ***@***.***> wrote: Would you mind rebasing this diff when you have a while? :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3906 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWFpGJ-GlNi3157_dbOFXiaKpnUs_ks5sKoLdgaJpZM4OEsZ-> .

thymikee · 2017-07-04T18:55:21Z

It passes locally, I've rebased and restarted the build, we'll see :)

bestander · 2017-07-04T19:01:55Z

Thanks, mate

…

On Tue, Jul 4, 2017 at 11:55 AM Michał Pierzchała ***@***.***> wrote: It passes locally, I've rebased and restarted the build, we'll see :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3906 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWMRFwBodTHAXgufq2GpW9RvPMhCAks5sKoqdgaJpZM4OEsZ-> .

thymikee · 2017-07-04T19:35:13Z

Tried rebuilding without cache, but still the same. Not sure why it happens. cc @SimenB @skovhus

skovhus · 2017-07-04T20:13:32Z

Sound weird that this affects the rollup build, but now I'm super happy that I added the eslint check. 😊

Have you looked into the es5 build for each package and see if they all contains un-transpiled code?

skovhus · 2017-07-04T20:16:01Z

@thymikee @bestander

jest-matchers/build-es5/index.js
  4593:1  error  Parsing error: The keyword 'const' is reserved

✖ 1 problem (1 error, 0 warnings)

Looking into the bundle reveals that ansi-styles@3.0.0 contains ES2015 code (e.g. const and for... of)... 😐

I guess this will happen as node library authors are targeting newer version of node.

bestander · 2017-07-04T20:39:37Z

Yes it fails locally if you clean repo from all node_modules in packages

…

On Tue, Jul 4, 2017 at 1:16 PM Kenneth Skovhus ***@***.***> wrote: @bestander <https://github.com/bestander> does it also fail locally? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3906 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWDd2TZN2UKeMAok5b8-BvtSqsTEhks5sKp2GgaJpZM4OEsZ-> .

skovhus · 2017-07-04T20:40:44Z

This patch solves the problem by also running Babel on node_modules (except for node_modules/babel-runtime that Babel ironically fails on with a A module cannot import itself error):

From b62040420421da46de054e7b9afc25649b45700d Mon Sep 17 00:00:00 2001
From: Kenneth Skovhus <kenneth.skovhus@gmail.com>
Date: Tue, 4 Jul 2017 22:39:04 +0200
Subject: [PATCH] Fix browser build (ansi-styles contains ES2015 code)

---
 scripts/browserBuild.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/browserBuild.js b/scripts/browserBuild.js
index ac43ca0..db1b4a2 100644
--- a/scripts/browserBuild.js
+++ b/scripts/browserBuild.js
@@ -20,7 +20,7 @@ const babelEs5Options = Object.assign(
   {},
   {
     babelrc: false,
-    exclude: 'node_modules/**',
+    exclude: 'node_modules/babel-runtime/**',
     plugins: [
       'syntax-trailing-function-commas',
       'transform-flow-strip-types',
-- 
2.10.2

@thymikee @bestander
But the underlying issues seems to be that we are upgrading dependencies in the packages (example ansi-styles@2 to ansi-styles@3) due to all the lock files being removed. See #3906 (comment)

skovhus · 2017-07-04T20:46:10Z

appveyor.yml

@@ -5,15 +5,17 @@ environment:
 install:
  - ps: Install-Product node $env:nodejs_version x64
  - node --version
-  - yarn --version
-  - yarn install
+  - curl -fsSL -o yarn.js https://github.com/yarnpkg/yarn/releases/download/v0.27.5/yarn-0.27.5.js


Wouldn't npm install --global yarn@0.27.5 be nicer here than node ./yarn.js?

Then we would need to be sure that npm global dir precedes appveyor's global Yarn installation in PATH.
I don't have a strong opinion on this and happy to change to anything that maintainers think is best.

To me it is just more standard to rely on a global yarn installation. I hope appveyour global Yarn installation will be overridden by a npm install -g.

So what should I do here, npm install -g?

skovhus · 2017-07-04T20:49:32Z

packages/babel-jest/yarn.lock

@@ -1,727 +0,0 @@
-# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.


Does this mean that each package dependencies are not locked by a yarn.lock file?

Ah, yes. 😎

Unlike Lerna Yarn hoists workspaces to the root node_modules.

skovhus · 2017-07-04T20:51:46Z

packages/jest-diff/yarn.lock

-  version "2.1.1"
-  resolved "https://registry.yarnpkg.com/ansi-regex/-/ansi-regex-2.1.1.tgz#c3b33ab5ee360d86e0e628f0468ae7ef27d654df"
-
-ansi-styles@^2.2.1:


This is the reason why the ES5 build fails as jest-diff used to use ansi-styles@2.x and now is uses whatever the global jest package.json defines (which is ansi-styles@3.0.0)...

Why don't we want these lock files anymore? This might cause other issues as all packages dependencies might have changed.

Just read you description again @bestander and some of the Yarn Workspace RFCs. I guess removing the packages lockfiles and node_modules is exactly what you means by

Unlike Lerna Yarn hoists workspaces to the root node_modules.

: )

cpojer · 2017-07-04T21:10:28Z

Can we whitelist only the node_moduels that are not ES5 compliant and transform those? I'd much rather compile only one or two that need it, rather than all of them.

skovhus · 2017-07-04T21:53:46Z

@cpojer a whitelist might be a moving target, but probably a better option. 👍🏻

This works:

-    exclude: 'node_modules/**',
+    exclude: 'node_modules/!(ansi-styles|remove-trailing-separator)/**',

remove-trailing-separator can be fixed by upgrading from 1.0.1 to 1.0.2, see darsain/remove-trailing-separator@6dd7a71

cpojer · 2017-07-05T08:10:09Z

Yeah let's do this. It's fine with me if we have to update this list from time to time.

bestander · 2017-07-05T18:41:13Z

lerna.json

-  "packages": [
-    "packages/*"
-  ]
+  "yarnWorkspaces": ["bootstrap"]


Waiting for the Lerna PR to merge first

bestander · 2017-07-05T19:00:50Z

Thanks a lot, @skovhus, this fixed the build.
Indeed it is great that we have es5 tests now.

The way rollup picks modules is a bit concerning though, it all goes down to what structure is inside the root node_modules and it is conforms with Node.js resolution, not for browser builds for each package in isolation

We might rethink how those jest-mock and jest-matchers are built.
They probably should do it in isolation with --flat mode.

codecov-io · 2017-07-05T19:43:59Z

Codecov Report

Merging #3906 into master will increase coverage by 0.26%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3906      +/-   ##
==========================================
+ Coverage   59.87%   60.13%   +0.26%     
==========================================
  Files         196      195       -1     
  Lines        6794     6736      -58     
  Branches        6        6              
==========================================
- Hits         4068     4051      -17     
+ Misses       2723     2682      -41     
  Partials        3        3

Impacted Files	Coverage Δ
packages/jest-config/src/index.js	`0% <0%> (-36.37%)`	⬇️
...ages/jest-config/src/reporter_validation_errors.js	`16.66% <0%> (-16.67%)`	⬇️
packages/jest-config/src/find_config.js	`0% <0%> (-15.16%)`	⬇️
packages/jest-config/src/defaults.js	`85.71% <0%> (-14.29%)`	⬇️
packages/jest-config/src/vendor/jsonlint.js	`0% <0%> (-5.73%)`	⬇️
packages/jest-config/src/utils.js	`73.33% <0%> (-3.75%)`	⬇️
packages/jest-config/src/normalize.js	`84.66% <0%> (-0.89%)`	⬇️
packages/jest-resolve-dependencies/src/index.js	`100% <0%> (ø)`	⬆️
packages/jest-config/src/valid_config.js	`100% <0%> (ø)`	⬆️
packages/jest-diff/src/constants.js	`100% <0%> (ø)`	⬆️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 286b877...3d63117. Read the comment docs.

cpojer · 2017-07-06T08:59:49Z

Are we good to go on this one? Is the latest version stable and rolled out so that people can use this feature?

thymikee · 2017-07-06T09:00:52Z

I was able to install yarn 0.27.5 through brew, so pretty sure it's rolled out 👍

thymikee · 2017-07-06T09:01:08Z

we can put "engines" entry in package.json though

skovhus · 2017-07-06T09:07:05Z

Are we good to go on this one? Is the latest version stable and rolled out so that people can use this feature?

A lot of dependencies might have been bumped in all packages as dependencies are resolved from outer package.json file now. But I guess this is fairly safe due to the test coverage we have.

bestander · 2017-07-07T01:10:54Z

@thymikee, "engines" probably won't do much, in latest npm it is not strict.

bestander · 2017-07-07T05:02:47Z

Updates Lerna to 2.0.0, it supports workspaces now

cpojer · 2017-07-07T14:00:14Z

And I'm getting another error about const. If you fix CI, happy if you merge this. Love the change.

bestander · 2017-07-07T19:21:11Z

Should be green now again.

facebook-github-bot · 2017-07-07T20:22:58Z

@bestander has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

cpojer · 2017-07-07T21:54:23Z

Woohoo, awesome work @bestander!

* enabled workspaces * updated lerna to 2.0.0 with workspaces support

github-actions · 2021-05-13T11:06:47Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

facebook-github-bot added the CLA Signed ✔️ label Jun 25, 2017

bestander mentioned this pull request Jun 27, 2017

Support Yarn workspaces to replace bootstrap command lerna/lerna#899

Merged

7 tasks

bestander mentioned this pull request Jul 1, 2017

Use rollup for browser builds #3532

Merged

skovhus reviewed Jul 4, 2017

View reviewed changes

bestander commented Jul 5, 2017

View reviewed changes

bestander added 2 commits July 6, 2017 21:49

enabled workspaces

fbf1b6c

updated lerna to 2.0.0 with workspaces support

93d3581

bestander added 2 commits July 7, 2017 14:06

enabled workspaces

517375e

updated lerna to 2.0.0 with workspaces support

b39383b

cpojer mentioned this pull request Jul 7, 2017

Auto detect and merge lockfile conflicts yarnpkg/yarn#3544

Merged

rarkins mentioned this pull request Jul 7, 2017

Yarn workspaces renovatebot/renovate#473

Closed

3 tasks

cpojer approved these changes Jul 7, 2017

View reviewed changes

bestander added 2 commits July 7, 2017 11:56

merge

de3fc76

merge and fix es5 build

3d63117

bestander merged commit e6704bf into jestjs:master Jul 7, 2017

SimenB mentioned this pull request Jul 8, 2017

Remove unused yarn.lock #3989

Merged

bestander mentioned this pull request Jul 17, 2017

Support unpublished lerna packages – ignore symlinked packages or not found failures yarnpkg/yarn#2863

Closed

richburdon mentioned this pull request Jul 18, 2017

Workspaces issue progress yarnpkg/yarn#3294

Closed

22 tasks

tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017

Integrated with Yarn workspaces (jestjs#3906)

d589d3f

* enabled workspaces * updated lerna to 2.0.0 with workspaces support

shawnbot mentioned this pull request Aug 29, 2017

[WIP] Update to lerna@2.1.1; use Yarn + workspaces primer/css#331

Closed

2 tasks

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

		@@ -1,727 +0,0 @@
		# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.

Integrated with Yarn workspaces #3906

Integrated with Yarn workspaces #3906

Conversation

bestander commented Jun 25, 2017 • edited Loading

bestander commented Jun 25, 2017

cpojer commented Jun 27, 2017

bestander commented Jun 27, 2017

thymikee commented Jun 27, 2017

cpojer commented Jun 27, 2017

bestander commented Jun 29, 2017

thymikee commented Jul 4, 2017

bestander commented Jul 4, 2017 via email

thymikee commented Jul 4, 2017

bestander commented Jul 4, 2017 via email

thymikee commented Jul 4, 2017

bestander commented Jul 4, 2017 via email

thymikee commented Jul 4, 2017

skovhus commented Jul 4, 2017 • edited Loading

skovhus commented Jul 4, 2017 • edited Loading

bestander commented Jul 4, 2017 via email

skovhus commented Jul 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skovhus Jul 4, 2017 • edited Loading

Choose a reason for hiding this comment

cpojer commented Jul 4, 2017

skovhus commented Jul 4, 2017 • edited Loading

cpojer commented Jul 5, 2017

Choose a reason for hiding this comment

bestander commented Jul 5, 2017 • edited Loading

codecov-io commented Jul 5, 2017 • edited Loading

Codecov Report

cpojer commented Jul 6, 2017

thymikee commented Jul 6, 2017

thymikee commented Jul 6, 2017

skovhus commented Jul 6, 2017

bestander commented Jul 7, 2017

bestander commented Jul 7, 2017

cpojer commented Jul 7, 2017

bestander commented Jul 7, 2017

facebook-github-bot commented Jul 7, 2017

cpojer commented Jul 7, 2017

github-actions bot commented May 13, 2021

bestander commented Jun 25, 2017 •

edited

Loading

skovhus commented Jul 4, 2017 •

edited

Loading

skovhus commented Jul 4, 2017 •

edited

Loading

skovhus commented Jul 4, 2017 •

edited

Loading

skovhus Jul 4, 2017 •

edited

Loading

skovhus commented Jul 4, 2017 •

edited

Loading

bestander commented Jul 5, 2017 •

edited

Loading

codecov-io commented Jul 5, 2017 •

edited

Loading