Change Yarn detection to look for lockfile

[backspace]

This is a naïve fix for #133. It would seemingly work to merge, but I’m not sure if more nuance is needed, or whether the change should extend further.

1. I made use of throw because it was the minimal change, since that’s what execFileSync would do when yarn --version failed. I could instead strip out the exception handling and directly use the simple boolean check of isFile().

2. The existing test for this doesn’t really exercise things fully, it mocks _runYarnCheck. Would it be worth using a true yarn.lock for testing?

3. What happens if a yarn.lock is detected but yarn itself isn’t present? Should the execFileSync perhaps remain? 🤔

Let me know your thoughts and I’m happy to improve this. Thanks for ember-try, it’s endlessly useful!

[kategengler]

Re: 1, I think removing the exception handling would be a nice cleanup if you have the time

Re: 2, Another test with an actual file for yarn.lock is a nice-to-have

Re: 3, I think failing if yarn is not installed but yarn.lock is committed is fine, but we need to add an "out" for people who for some reason have a yarn.lock but would like to use npm. A configuration option to force use of npm or yarn, maybe. (But I wouldn't necessarily block this PR on that)

[kellyselden]

Can this check for yarn installed as well as yarn.lock? That way CI servers that don't have yarn won't start breaking with this change.

[kategengler]

@backspace Thanks for working on this. After thinking it through and chatting with @kellyselden and @rwjblue, we'd like to make use of yarn be only through opt-in via the config (useYarn: true). We'll also warn if the user has a yarn.lock and yarn on the system and does not have useYarn: true in the config (but only if there's a scenario with npm dependencies).

Are you interested/willing/do have time to take this PR in that direction?

[backspace]

I’m into replacing this with the plan you detailed. Do you think that maybe the presence of yarn.lock alone without useYarn: true should suggest useYarn (regardless of whether yarn exists as a command), since presumably if they have the file they probably want to use it and should make sure it’s installed?

I should be able to try getting into this direction later this weekend or during the upcoming week.

I did have a mysterious CI failure in my work on this to date:

  1) tryEach with stubbed dependency manager passes along timeout options to run:
     Error: timeout of 1200ms exceeded. Ensure the done() callback is being called in this test.

There’s a comment within the test specifically addressing it:

        // With stubbed dependency manager, timing out is warning for accidentally not using the stub

It was confusing to me because I didn’t think I had changed anything related to using the stub. But hopefully this error will disappear as I change things around with the current plan 🤞

[kategengler]

I think that approach w/ just yarn.lock is reasonable, since it'll just be a warning. Thanks for continuing this work even though we changed the plan out from under you.

Re: CI failure, most of the time that comment is true. I haven't seen that test be flaky before, but it's possible. If it doesn't go away, I'll look into it more.

[backspace]

Re: CI failure, most of the time that comment is true. I haven't seen that test be flaky before, but it's possible. If it doesn't go away, I'll look into it more.

hah I totally believe it, I probably changed something I didn’t realise would affect it.

Regarding this:

we'd like to make use of yarn be only through opt-in via the config (useYarn: true)

Am I correct in understanding that this is in the ember-try.js file, like if it were in the dummy one:

module.exports = {
  scenarios: [
    {
      name: 'test1',
      bower: {
        dependencies: {
          ember: '1.10.0'
        }
      }
    },
    {
      name: 'test2',
      command: 'ember test',
      npm: {
        useYarn: true,
        dependencies: {
          'ember-feature-flags': '3.0.0'
        }
      }
    }
  ]
};

Or is it meant to go elsewhere?

[kellyselden]

@backspace It would be top level outside scenarios:

module.exports = {
  useYarn: true,
  scenarios: [
    {
      name: 'test1',
      bower: {
        dependencies: {
          ember: '1.10.0'
        }
      }
    },
    {
      name: 'test2',
      command: 'ember test',
      npm: {
        dependencies: {
          'ember-feature-flags': '3.0.0'
        }
      }
    }
  ]
};

[backspace]

ah, makes sense, thank you!

[backspace]

I have some progress on this. Here’s a screenshot of the warning:

The wording can of course be adjusted. Should it be coloured differently? I see elsewhere there’s task.ui.writeLine used to write coloured lines out to the terminal, but I wasn’t sure how to access that in the npm adapter.

I added useYarn: true to the smoke test’s ember-try config; it seemed to make sense because it looks like the smoke test is only run on Travis, which has a Yarn installation in the before_install.

It feels a bit unsatisfying to have no testing around the yarn.lock detection when useYarn isn’t true but it also seemed weird to assert on a particular line being output to the log… 🤔

If there’s anything you think should be added or changed, let me know!

ETA: hmm an obvious oversight is updating the README but I suspect there are other things too 😀

[kategengler]

Re: task.ui.writeLine is definitely preferred over console.log. You'd have to pass in ui to the DependencyManagerAdapter. Alternatively, if you moved the yarn check to setup, you could pass it in just to that method from the ScenarioManager.

Re: testing for log, I've done it elsewhere. Here is an example: https://github.com/ember-cli/ember-try/blob/master/test/tasks/try-each-test.js#L138 Alternatively, you could refactor the check into a method that returns based on whether the yarn check passed or failed, and only write the console output based on that. It would allow you to unit test the yarn check without testing the output.

Also, 😀, thanks for the work and the detailed writeup!

[backspace]

Thanks for the example of log testing and general ongoing input 😀

What do you think of the Yarn check happening in tasks/try-each.js? I’m considering this because it seems like it really only needs to happen once, rather than for every scenario, and it probably would be best early on so it’s more noticeable. It looks like try:one delegates to try:each under the surface, so it would be covered by this too.

[kategengler]

The DependencyManagerAdapter should only be created once whether running try:one or try:each. I think it's better in the npm adapter because the check should only happen when there are scenarios with npm changes.

[backspace]

Okay, understood!

Here’s a screenshot of the warning with writeLine:

I updated one of the tasks/try-each-test.js tests to create a yarn.lock and added an expectation for the warning text in the output. I’ve now also updated the README. Maybe this is getting closer to being ready?

[kategengler]

Is this preferable to fs.writeFileSync?

[backspace]

hah! I just copied from Stack Overflow 😳 but I guess writeFileSync is newer than that answer! I’ll replace with that. 😀

hmm well the answer is from 2012 but that method has been around since Node 0.1.29 🤔 … I just searched for “node touch file” and used that. anywayyyyy

[kategengler]

@backspace Just had the one question in the test, otherwise LGTM and I'll merge once you reply. Thanks again for all the work & updates.

[kategengler]

Thanks for the change! I'll merge as soon as CI passes.

[backspace]

Excellent, thanks for the guidance!

[mansona]

Great work everyone 🎉 I love it when I go looking for something I need and someone has put the work in 😄

How do I get this right now? Looking at the module I'm working on it doesn't seem to depend on ember-try directly, I'm assuming that's because now it's a default dependency of ember-cli?

If I added the the github version of ember-try to my package.json would this work or do I need to wait until there is a version released and ember-cli depends on that version?

Not trying to rush anyone just curious how I can make use of this new awesome 😂

[backspace]

When I was trying workarounds, I found that putting ember-try directly in my package.json did cause it to override the Ember CLI version. So you could try that, with the branch specified as master? At least until a version is released.

[mansona]

thanks @backspace that did indeed work 👍

[kellyselden]

Released in v0.2.17