feat: add support for yarn installations #560

SimenB · 2018-04-15T12:01:06Z

I'd really like to add Jest to CITGM, but the hard requirement on npm means that's a no-go (Jest uses yarn workspaces). I think I read somewhere that npm was going to add workspaces support, but seeing as I can't find a reference to it, I might have imagined it.

This is a first go at adding support for Yarn as package manager instead of npm in CITGM.

While doing this I realized that #415 just covers adding it to $PATH, not necessarily using it (and that should probably still be done). I've already done the work here though, so why not open up a PR? 🙂 I've held off on adding tests as there's a chance this is rejected, though.

2 times Jest has had failing tests on Node master, with one of them being caught before node 9 was released, and one making it into the release. See nodejs/node#16322 and nodejs/node#19607.

/cc @cpojer

Checklist

npm test passes
tests are included
documentation is changed or added(https://github.com/nodejs/citgm/blob/master/CONTRIBUTING.md)

SimenB · 2018-04-15T12:10:19Z

Yeah, CI fails since yarn is not added to $PATH. Thoughts on the cleanest way of doing that?

cpojer · 2018-04-15T16:13:46Z

Wow this is really cool! Hope that this will get accepted. Is there anything I can do to help? :)

MylesBorins · 2018-04-15T16:17:04Z

I'll review this when I get back in to work tomorrow. We will need to work with @nodejs/build to get yarn into our ci as well

…

On Sun, Apr 15, 2018, 12:13 PM Christoph Nakazawa ***@***.***> wrote: Wow this is really cool! Hope that this will get accepted. Is there anything I can do to help? :) — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#560 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV8RN795NsPIc0Zao84pnsvbONlxPks5to3G7gaJpZM4TVci2> .

gibfahn · 2018-04-15T20:40:51Z

We will need to work with @nodejs/build to get yarn into our ci as well

Looking at the yarn installation docs it looks like it's just a case of extracting a tarball and adding to the path.

I'd rather not install yarn globally on every machine if possible, so just adding the two lines to the CI jobs sounds like the way to go for me.

EDIT: Realised I already said this in #415

gibfahn · 2018-04-15T20:43:10Z

This PR LGTM once it has tests.

SimenB · 2018-04-15T20:48:38Z

Opened up #561 installing yarn on CI (separate as I expect this PR to take a bit of time, while yarn installed on CI is standalone)

gibfahn · 2018-04-15T20:54:28Z

Opened up #561 installing yarn on CI (separate as I expect this PR to take a bit of time, while yarn installed on CI is standalone)

I don't have a huge issue with doing that, but what's the benefit to having that land sooner? Presumably it's not usable before this PR lands.

SimenB · 2018-04-15T20:54:58Z

To unblock #478

SimenB · 2018-04-19T18:51:14Z

@MylesBorins no objections beyond missing tests? If so, I'll try to get some written this weekend or early next week 🙂

SimenB · 2018-04-19T22:31:46Z

Thanks for the review! Seeing as Jest uncovered nodejs/node#20157 as well, I'll try to prioritise work on this PR so we can get Jest into CITGM.

MylesBorins · 2018-04-19T23:04:40Z

If you want to pair on a refactor of this pr lmk 😁

…

On Thu, Apr 19, 2018, 6:31 PM Simen Bekkhus ***@***.***> wrote: Thanks for the review! Seeing as Jest uncovered nodejs/node#20157 <nodejs/node#20157> as well, I'll try to prioritise work on this PR so we can get Jest into CITGM. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#560 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV1Yoyvj6UoqwgblDpmSqZ0gNIHAMks5tqRBTgaJpZM4TVci2> .

SimenB · 2018-04-19T23:06:23Z

Feel free to work on it! I'll be travelling for work this weekend, so I won't have any time until next week anyways 🙂

BridgeAR · 2018-05-20T21:47:47Z

@SimenB would you be so kind and have another loop at the comments? :-) having yarn support would IMO be really helpful.

SimenB · 2018-10-22T21:38:51Z

Jest has actually passed Mocha now as the most downloaded test framework from npm, so I'll try to find the time to get this up to speed within the next few days.

SimenB · 2018-10-23T08:26:07Z

Rebased and pushed changes based on feedback (except for template strings and the bind thing).

I'll open up a separate PR adding Prettier and template strings to avoid the style nits 🙂

.travis.yml

targos · 2018-10-23T10:48:10Z

Is this ready for review?

SimenB · 2018-10-23T14:14:59Z

Yes, except for a missing test this is good to go. I'll add a test today or tomorrow

SimenB · 2018-10-24T10:25:19Z

I've added tests now. However, I hit a snag.

On mac, of you install yarn through the officially recommended way (brew), the yarn binary is a shell script, not a JS file. I "fixed" this by spawning the binary directly (ede9d29) and not via node. This however means that the test with fake-node fails. Thoughts?

I skipped the failing tests in the last commit.

Brew code: https://github.com/Homebrew/homebrew-core/blob/851bec0243ba6e5e722fedf0de172a53ff1d9299/Formula/yarn.rb#L13-L18

Results in the following /usr/local/bin/yarn:

#!/bin/bash
PREFIX="/usr/local" NPM_CONFIG_PYTHON="/usr/bin/python" exec "/usr/local/Cellar/yarn/1.10.1/libexec/bin/yarn.js" "$@"

test/yarn/test-yarn-install.js

lib/package-manager/test.js

test/npm/test-npm-test.js

test/yarn/test-yarn-install.js

test/yarn/test-yarn-test.js

targos · 2018-10-24T11:31:32Z

This LGTM, but we have to make sure that the CI machines used for CITGM jobs have yarn installed and in the PATH before we merge the PR.

MylesBorins · 2018-10-24T14:06:48Z

Are we able to include yarn in the package.json and use that version?

…

On Wed, Oct 24, 2018, 7:31 AM Michaël Zasso ***@***.*** wrote: This LGTM, but we have to make sure that the CI machines used for CITGM jobs have yarn installed and in the PATH before we merge the PR. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#560 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV4Jr9I9h_reRcEVPZqU8J7MpPWOAks5uoE-VgaJpZM4TVci2> .

SimenB · 2018-10-24T17:18:55Z

Are we able to include yarn in the package.json and use that version?

That should work, yeah!

SimenB · 2018-11-21T15:17:03Z

@MylesBorins thanks for the thorough review! I think I've addressed your points now (and rebased)

lib/citgm.js

SimenB · 2018-11-21T15:22:58Z

test/npm/test-npm-install.js

 test('npm-install: setup', function (t) {
-  t.plan(7);
+  t.plan(8);
+  packageManager.getPackageManagers((e, res) => {


not super clean, but allows me to reuse the implementation.

MylesBorins

Some small nits in line but nothing I'd block over

Thanks for all the hard work!

edit: this LGTM is dependent on the test suite being green

lib/citgm.js

MylesBorins · 2018-11-21T16:04:03Z

lib/package-manager/get-executable.js

+module.exports = function getExecutable(binaryName, next) {
+  if (binaryName === 'yarn') {
+    // Use `npm-which` for yarn to get the locally version
+    npmWhich(binaryName, next);


is there a reason we can't use npmWhich for both npm and yarn?. If that's the case and it is pass through could we not remove this script altogether?

is there a reason we can't use npmWhich for both npm and yarn?

npm-which is just for binaries installed in node_modules. Not sure if it finds globally installed stuff?

Could potentially add npm as a dep as well, then it won't rely on any globals beyond node. Maybe not worth it?

If that's the case and it is pass through could we not remove this script altogether?

IDK about the windows thing below. Would that just work ™️? I don't have access to a windows machine to test

Let's keep it just how it is for now, this makes sense. I was more confused by the usage above

lib/package-manager/index.js

SimenB · 2018-11-21T16:22:13Z

Fixed the failing test and addressed the last feedback except for one open question

SimenB · 2018-11-21T16:46:49Z

BTW, might make sense to activate auto-cancellation of redundant builds: https://travis-ci.com/nodejs/citgm/pull_requests. Currently my last 2 commits are both queued, the second to last could (should) be cancelled

targos · 2018-11-21T17:03:53Z

@SimenB done. I also cancelled the second to last job.

SimenB · 2018-11-22T11:54:02Z

CI is green and feedback addressed. Is this good to go? 🙂

rvagg · 2018-11-22T11:55:45Z

I'm not really following this, sorry, but if this needs yarn on citgm infra then could someone please open an issue on nodejs/build if there isn't one for it?

SimenB · 2018-11-22T11:56:50Z

I've added yarn as a dependency, so it's installed that way. Shouldn't need to update any CI env

targos · 2018-11-24T13:59:14Z

CI: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-continuous-integration-pipeline/82/console

targos · 2018-11-24T14:05:25Z

Failures seem unrelated. I opened nodejs/build#1601

targos · 2018-11-24T14:07:37Z

I scheduled https://ci.nodejs.org/job/citgm-continuous-integration/144/ to check if it also fails for the same reason on master.

Edit: confirmed

This comment was marked as off-topic.

Sign in to view

SimenB mentioned this pull request Apr 23, 2018

assert: add rawMessage to error messages nodejs/node#19723

Closed

4 tasks

Trott mentioned this pull request Jul 20, 2018

10.7.0 broke the Yarn tests nodejs/node#21907

Closed

richardlau mentioned this pull request Oct 15, 2018

lib: make the global console [[Prototype]] an empty object nodejs/node#23509

Closed

3 tasks

SimenB mentioned this pull request Oct 23, 2018

chore: use template strings #611

Closed

1 task

This comment has been minimized.

Sign in to view

SimenB commented Oct 23, 2018

View reviewed changes

.travis.yml Outdated Show resolved Hide resolved

SimenB commented Oct 24, 2018

View reviewed changes

test/yarn/test-yarn-install.js Outdated Show resolved Hide resolved

targos reviewed Oct 24, 2018

View reviewed changes

lib/package-manager/test.js Outdated Show resolved Hide resolved

test/npm/test-npm-test.js Outdated Show resolved Hide resolved

test/yarn/test-yarn-install.js Outdated Show resolved Hide resolved

test/yarn/test-yarn-test.js Outdated Show resolved Hide resolved

SimenB added 9 commits November 21, 2018 15:12

remove custom test script

fb3ea5a

fix tests

ede6a95

remove uknown flag from yarn call

99df018

stringify buffers in test

4e4cfa6

tweak assertion

6829ba8

chore: rename file

013b093

rename exported function

8f7795b

simplify calling pkg manager install and test

261d246

cache package manager binary lookup

9bb22dc

SimenB commented Nov 21, 2018

View reviewed changes

lib/citgm.js Outdated Show resolved Hide resolved

SimenB commented Nov 21, 2018

View reviewed changes

MylesBorins approved these changes Nov 21, 2018

View reviewed changes

SimenB added 2 commits November 21, 2018 17:20

fix failing test

792e673

PR feedback

669492e

lint

04964a3

targos merged commit 44249dc into nodejs:master Nov 24, 2018

SimenB deleted the yarn branch November 24, 2018 14:33

SimenB mentioned this pull request Dec 8, 2018

win: fix spawning npm #635

Closed

2 tasks

This was referenced Mar 7, 2019

esm: fixup process Proxy to handle toString correctly nodejs/node#25963

Closed

Add Jest to lookup #684

Closed

feat: add support for yarn installations #560

feat: add support for yarn installations #560

Conversation

SimenB commented Apr 15, 2018 • edited Loading

Checklist

SimenB commented Apr 15, 2018 • edited Loading

cpojer commented Apr 15, 2018

MylesBorins commented Apr 15, 2018 via email

gibfahn commented Apr 15, 2018 • edited Loading

gibfahn commented Apr 15, 2018

SimenB commented Apr 15, 2018 • edited Loading

gibfahn commented Apr 15, 2018

SimenB commented Apr 15, 2018

SimenB commented Apr 19, 2018

This comment was marked as off-topic.

SimenB commented Apr 19, 2018

MylesBorins commented Apr 19, 2018 via email

SimenB commented Apr 19, 2018

BridgeAR commented May 20, 2018

SimenB commented Oct 22, 2018

SimenB commented Oct 23, 2018

This comment has been minimized.

targos commented Oct 23, 2018

SimenB commented Oct 23, 2018

SimenB commented Oct 24, 2018

targos commented Oct 24, 2018

MylesBorins commented Oct 24, 2018 via email

SimenB commented Oct 24, 2018

SimenB commented Nov 21, 2018

SimenB Nov 21, 2018

Choose a reason for hiding this comment

MylesBorins left a comment • edited Loading

Choose a reason for hiding this comment

MylesBorins Nov 21, 2018

Choose a reason for hiding this comment

SimenB Nov 21, 2018

Choose a reason for hiding this comment

MylesBorins Nov 21, 2018

Choose a reason for hiding this comment

SimenB commented Nov 21, 2018

SimenB commented Nov 21, 2018 • edited Loading

targos commented Nov 21, 2018

SimenB commented Nov 22, 2018

rvagg commented Nov 22, 2018

SimenB commented Nov 22, 2018

targos commented Nov 24, 2018

targos commented Nov 24, 2018

targos commented Nov 24, 2018 • edited Loading

SimenB commented Apr 15, 2018 •

edited

Loading

SimenB commented Apr 15, 2018 •

edited

Loading

gibfahn commented Apr 15, 2018 •

edited

Loading

SimenB commented Apr 15, 2018 •

edited

Loading

MylesBorins left a comment •

edited

Loading

SimenB commented Nov 21, 2018 •

edited

Loading

targos commented Nov 24, 2018 •

edited

Loading