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

Integrated with Yarn workspaces #3906

Merged
merged 6 commits into from
Jul 7, 2017
Merged

Integrated with Yarn workspaces #3906

merged 6 commits into from
Jul 7, 2017

Conversation

bestander
Copy link
Contributor

@bestander bestander commented Jun 25, 2017

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:

  1. 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.

  1. 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
  1. 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
Copy link
Contributor Author

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

@cpojer
Copy link
Member

cpojer commented Jun 27, 2017

This is super sweet @bestander!

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

@bestander
Copy link
Contributor Author

@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
Copy link
Collaborator

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
Copy link
Member

cpojer commented Jun 27, 2017

Yep, I'm fine with it also.

@bestander
Copy link
Contributor Author

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

@thymikee
Copy link
Collaborator

thymikee commented Jul 4, 2017

@bestander is yarn stable now?

@bestander
Copy link
Contributor Author

bestander commented Jul 4, 2017 via email

@thymikee
Copy link
Collaborator

thymikee commented Jul 4, 2017

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

@bestander
Copy link
Contributor Author

bestander commented Jul 4, 2017 via email

@thymikee
Copy link
Collaborator

thymikee commented Jul 4, 2017

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

@bestander
Copy link
Contributor Author

bestander commented Jul 4, 2017 via email

@thymikee
Copy link
Collaborator

thymikee commented Jul 4, 2017

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

@skovhus
Copy link
Contributor

skovhus commented Jul 4, 2017

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
Copy link
Contributor

skovhus commented Jul 4, 2017

@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
Copy link
Contributor Author

bestander commented Jul 4, 2017 via email

@skovhus
Copy link
Contributor

skovhus commented Jul 4, 2017

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)

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yes. 😎

Unlike Lerna Yarn hoists workspaces to the root node_modules.

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

ansi-styles@^2.2.1:
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@skovhus skovhus Jul 4, 2017

Choose a reason for hiding this comment

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

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
Copy link
Member

cpojer commented Jul 4, 2017

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
Copy link
Contributor

skovhus commented Jul 4, 2017

@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
Copy link
Member

cpojer commented Jul 5, 2017

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

lerna.json Outdated
"packages": [
"packages/*"
]
"yarnWorkspaces": ["bootstrap"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Waiting for the Lerna PR to merge first

@bestander
Copy link
Contributor Author

bestander commented Jul 5, 2017

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
Copy link

codecov-io commented Jul 5, 2017

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Member

cpojer commented Jul 6, 2017

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

@thymikee
Copy link
Collaborator

thymikee commented Jul 6, 2017

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

@thymikee
Copy link
Collaborator

thymikee commented Jul 6, 2017

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

@skovhus
Copy link
Contributor

skovhus commented Jul 6, 2017

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
Copy link
Contributor Author

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

@bestander
Copy link
Contributor Author

Updates Lerna to 2.0.0, it supports workspaces now

@cpojer
Copy link
Member

cpojer commented Jul 7, 2017

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

@bestander
Copy link
Contributor Author

Should be green now again.

@facebook-github-bot
Copy link
Contributor

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

@bestander bestander merged commit e6704bf into jestjs:master Jul 7, 2017
@cpojer
Copy link
Member

cpojer commented Jul 7, 2017

Woohoo, awesome work @bestander!

@SimenB SimenB mentioned this pull request Jul 8, 2017
tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
* enabled workspaces

* updated lerna to 2.0.0 with workspaces support
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants