-
-
Notifications
You must be signed in to change notification settings - Fork 298
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
Use Yarn when available #114
Use Yarn when available #114
Conversation
I think we should automatically use yarn if a |
@SamVerschueren what do you think about having the flag and also checking for the presence of EDIT: I just realized that it would be better to just just use |
cli.js
Outdated
@@ -48,6 +50,7 @@ Promise | |||
|
|||
return options; | |||
}) | |||
.then(options => Object.assign({}, options, {noYarn: noYarn(options)})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check should be done in index.js, not here.
index.js
Outdated
@@ -33,6 +33,7 @@ module.exports = (input, opts) => { | |||
opts.cleanup = false; | |||
} | |||
|
|||
const manager = opts.noYarn ? 'npm' : 'yarn'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const bin = opts.yarn ? 'yarn' : 'npm';
And add a default value to line 28, using the hasYarn
check.
cli.js
Outdated
@@ -19,6 +20,7 @@ const cli = meow(` | |||
--no-cleanup Skips cleanup of node_modules | |||
--yolo Skips cleanup and testing | |||
--tag Publish under a given dist-tag | |||
--no-yarn Do not use yarn (even if yarn.lock is present) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--no-yarn Don't use Yarn when available
index.js
Outdated
@@ -25,14 +26,16 @@ module.exports = (input, opts) => { | |||
input = input || 'patch'; | |||
|
|||
opts = Object.assign({ | |||
cleanup: true | |||
cleanup: true, | |||
yarn: !opts.noYarn && hasYarn() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yarn: opts.yarn !== false && hasYarn()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just realized that --no-yarn
gets parsed as opts.yarn
, was sure it was parsed as opts.noYarn
.
CI will fail anyways because opts
is undefined
on the tests. Will check why that happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, actually, should be: yarn: opts && opts.yarn !== false && hasYarn()
index.js
Outdated
@@ -57,8 +60,8 @@ module.exports = (input, opts) => { | |||
task: () => del('node_modules') | |||
}, | |||
{ | |||
title: 'Installing dependencies', | |||
task: () => exec('npm', ['install']) | |||
title: `Installing dependencies${opts.yarn ? ' (using yarn)' : ''}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(using yarn)
=> using Yarn
@sindresorhus since |
index.js
Outdated
@@ -57,8 +60,8 @@ module.exports = (input, opts) => { | |||
task: () => del('node_modules') | |||
}, | |||
{ | |||
title: 'Installing dependencies', | |||
task: () => exec('npm', ['install']) | |||
title: `Installing dependencies${opts.yarn ? ' using yarn' : ''}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice the capitalization in my previous comment: yarn => Yarn
Yes |
Just wondering about the ability of running |
@SamVerschueren I removed the |
Oh sweet. Totally missed that! Edit: Probably because of ⏬ |
index.js
Outdated
@@ -25,14 +26,16 @@ module.exports = (input, opts) => { | |||
input = input || 'patch'; | |||
|
|||
opts = Object.assign({ | |||
cleanup: true | |||
cleanup: true, | |||
yarn: opts && opts.yarn !== false && hasYarn() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is wrong. Because if I ran np --yarn
, this statement will be overridden with true
and hasYarn()
has no effect.
It should be
opts = Object.assign({
cleanup: true
}, opts, {
yarn: opts && opts.yarn !== false && hasYarn()
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, updated 🙃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have one last concern regarding the check. Currently it doesn't check if the yarn
command is even available. For instance, a project that I don't own might have a lockfile. If I'm able to publish to npm, it will try to do that with Yarn which I might have not installed. Could we check that?
Yes that makes sense! I'm not quite sure how to do it though 🤔 |
Maybe with node-which? Although there might a possibility @sindresorhus has a package for that 😆 ... |
Would be better to just try Yarn and handle the error if it doesn't exist. When it doesn't exists, |
That makes sense, will add it on Sunday. |
@gillchristian Happy xmas and new year :) Any chance you would be able to finish this up? |
Thanks, happy holidays to you too! Yes, I've been kind of busy, sorry for the delay! Will try to finish soon 😄 |
index.js
Outdated
module.exports = (input, opts) => { | ||
input = input || 'patch'; | ||
|
||
opts = Object.assign({ | ||
cleanup: true, | ||
publish: true | ||
}, opts, { | ||
yarn: opts && opts.yarn !== false && hasYarn() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not checking for yarn
anymore since we are defaulting to it, and falling back when is not found.
index.js
Outdated
@@ -22,14 +21,22 @@ const exec = (cmd, args) => { | |||
).filter(Boolean); | |||
}; | |||
|
|||
const install = (bin, useYarn) => () => execa(bin, ['install']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the first time I use execa
, so there might be a better way of doing this 🙃
Also I would like to display a message, when falling back to npm
, any suggestions on how @sindresorhus @SamVerschueren ?
Something like:
✔ Prerequisite check
✔ Git
✔ Cleanup
⠦ Installing dependencies using Yarn
Yarn not installed, falling back to npm
✔ Running tests
✔ Bumping version
✔ Publishing package
✔ Pushing tags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant to use exec()
here and below.
Also I would like to display a message, when falling back to npm, any suggestions on how
@SamVerschueren Any suggestion how to achieve this?
Maybe instead, we would create a new dynamic task if the Yarn one fails.
Example:
✔ Prerequisite check
✔ Git
✔ Cleanup
- Installing dependencies using Yarn
Yarn not installed, falling back to npm
⠦ Installing dependencies
✔ Running tests
✔ Bumping version
✔ Publishing package
✔ Pushing tags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if that's possible at the moment. The only thing you could do is work with the skipped functionality. The downside of this is that it decides upfront if the task is skipped or not. So if you're in the task
function, it's not possible to skip it afterwards because something failed.
What you could do however is running yarn --version
in the skip
function and detect if that works. If not, you can show a skipped message. So that can be solved this way, something like this.
{
title: 'Installing dependencies using Yarn',
skip: () => execa('yarn', '--version')
.then(() => false)
.catch(() => 'Yarn not installed, falling back to npm')
},
task: () => execa('yarn')
}
About adding a new task dynamically, never tried it, I don't think it would work. What we can do for now is showing the same line but with npm
and skip if the dependencies are already installed with Yarn.
Solution 1
{
title: 'Installing dependencies using Yarn',
skip: () => execa('yarn', '--version')
.then(() => false)
.catch(() => 'Yarn not installed, falling back to npm')
},
task: ctx => {
ctx.yarn = true;
return execa('yarn');
}
},
{
title: 'Installing dependencies using npm',
skip: ctx => {
if (ctx.yarn) {
return 'Dependencies already installed with Yarn';
}
},
task: () => execa('npm', ['install'])
}
But to be honest, that would look like crap. People might think we're going to install dependencies twice.
Solution 2
Using observables to show a message
{
title: 'Installing dependencies',
task: ctx => {
return new Observable(observer => {
execa('yarn')
.then(() => observer.complete())
.catch(() => {
observer.next('Yarn not installed, falling back to npm');
return execa('npm', ['install'])
.then(() => observer.complete())
.catch(err => observer.error(err));
});
});
}
}
Solution 3
Add something like a visible
or disabled
property to Listr to dynamically show and hide tasks. That way we could use the skip
function to detect if yarn
is installed. If not, dynamically show the next task being Installing dependencies via npm
.
The best option for now would be 2 I guess. But we will loose the execa
output that is now generated because of npm install
. So if we want to fix this on the long run, I will have to add new functionality to Listr.
Please provide me with feedback. If you have other solutions or needs that you want in order to solve this in the most elegant way possible, don't hesitate to throw it out. These are just the first things that come in my mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comprehensive reply Sam. I too think 2
would be the best solution right now, but would like to see 3
be possible eventually.
Add something like a visible or disabled property to Listr to dynamically show and hide tasks. That way we could use the skip function to detect if yarn is installed.
Sounds useful, but why not also make skip
dynamic? You're pretty much treating the terminal as a canvas anyways, so anything could potentially be dynamic.
@SamVerschueren Could you open an issue about this on Listr, so we could continue this discussion there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not also make skip dynamic?
Can you elaborate on what you mean with this? skip
accepts a promise as well, so in that sense it is dynamic. But maybe you're hinting at something else :).
Could you open an issue about this on Listr, so we could continue this discussion there?
Jup, I will. Just want to clarify the skip
part first :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on what you mean with this? skip accepts a promise as well, so in that sense it is dynamic. But maybe you're hinting at something else :).
You said:
So if you're in the task function, it's not possible to skip it afterwards because something failed.
I'd like that to be possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha. Makes sense. I'll create two issues for these features and we can continue the discussion. I can probably work on this tomorrow so it might be better to wait a little longer to finish this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finally got into implementing these features 🎉 (released in 0.10.0
). I even used the yarn
with npm
fallback as example of the feature. https://github.com/SamVerschueren/listr#enabling-tasks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! I will look into it and add it here.
Sorry for the delays btw, been really busy 😬
index.js
Outdated
@@ -21,19 +21,30 @@ const exec = (cmd, args) => { | |||
).filter(Boolean); | |||
}; | |||
|
|||
const install = (bin, useYarn) => () => execa(bin, ['install']) | |||
.then(c => c, e => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant .catch()
here. And use err
, not e
.
And don't include the yarn.lock file. |
Really looking forward to this feature! @gillchristian Let me know if you don't have enough time for the modifications, I'd be more than happy to help. |
Yes @jonathandelgado, I'm glad if you can help 😄 I will be able to work on it next week, some stuff came up this week so my spare time wasn't there. |
@gillchristian Sounds good, working on it right now. Scanning the PR, it looks like there was a previous discussion on using yarn only if I would assume keep the previous behavior in which the default is based on Once I hear back on that functionality, I'll push changes to https://github.com/jonathandelgado/np and we can go from there. |
You're right, we used to have the |
@jonathandelgado Yes, we removed And for those that do not want to use |
@gillchristian I think something went wrong in the communication. We still need to check if a |
I have to agree with @SamVerschueren, I think the best course of action is defaulting based on the existence of a Yarn lock. I pushed the following changes to jonathandelgado/np#add-yarn-option - We can either merge them here, or I can create a separate PR.
I tested the following conditions:
I have only one issue with the current implementation. If I have a dependency I cannot install, for instance a OSX only module on the Windows platform, it also triggers a fail-over. |
Yes, I like that approach too. I guess I just misunderstood the discussion. @jonathandelgado regarding the PR, I'm ok with both options. |
@jonathandelgado The PR looks good.
I'm not sure about this one to be honest. This will produce a |
@gillchristian If you could merge the two and squash after I fix the last problem, that would be great. I would prefer to keep all the discussion in a single PR for clarity's sake 😄 @SamVerschueren Good call, I removed that flag.
I thought more about this issue, I'm becoming more convinced we should really just throw the error and let the user deal with it rather than attempting an npm fail-over. If there is a lock file and they don't have Yarn installed (causing a fail-over), this might silently break modules with non-pinned versions, leading to issues down the road. A user should explicitly state if they want to abandon the lock using the Let me know what you think and I will implement the change. Thank you all for the speedy feedback! |
I went ahead and applied this behavior to my branch. I think the user should explicitly state they want to abandon the lock using @gillchristian Feel free to merge from jonathandelgado/np#add-yarn-option, it is really for review. Thanks! |
ping @gillchristian :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small remarks
index.js
Outdated
}); | ||
publish: true, | ||
yarn: hasYarn() | ||
}, opts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not entirely correct. If they force yarn by using np --yarn
while there is no lockfile, it will create a lockfile and I think we should prevent that. Instead I would do something like
opts = Object.assign({
cleanup: true,
publish: true
}, opts, {
yarn: opts && opts.yarn && hasYarn()
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would lead to confusing behavior. If the user wants to switch to Yarn, but has yet to generate a lockfile, it would seem as if this library isn't working.
I think the current implementation is fine, yet if you really want to ensure a lockfile is present, we should throw an error and alert the user what the issue is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you would force a lockfile it would fail downstream because it tries to push the tags but not everything is committed yet... So I vote against the possibility of enforcing a lockfile because it would fail...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok, I see the problem you are describing, I thought it did it in a different order.
I would propose adding something like the following for greater transparency.
if (!hasYarn() && opts.yarn){
throw new Error('Tried to use Yarn without a yarn.lock file.');
}
Do you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes that makes sense I guess. Maybe something like
Could not use Yarn without
yarn.lock
file
index.js
Outdated
task: install(bin, opts.yarn) | ||
title: 'Installing dependencies using Yarn', | ||
enabled: () => opts.yarn === true, | ||
task: () => exec('yarn', ['install']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should implement a fallback as shown in the example of listr. If yarn
fails because the binary is not available, we should fall back to npm
as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mentioned this issue a few times in the PR, i'll quote some relevant sources for context:
If I have a dependency I cannot install, for instance a OSX only module on the Windows platform, it also triggers a fail-over. execa gives an ENOENT code whether the command could not be found or the installation failed (on Windows at least). It appears to me the only difference in output is the stderr. I could create a regex to differentiate the two, but that seems hacky.
I thought more about this issue, I'm becoming more convinced we should really just throw the error and let the user deal with it rather than attempting an npm fail-over. If there is a lock file and they don't have Yarn installed (causing a fail-over), this might silently break modules with non-pinned versions, leading to issues down the road. A user should explicitly state if they want to abandon the lock using the --no-yarn flag.
I went ahead and applied this behavior to my branch. I think the user should explicitly state they want to abandon the lock using --no-yarn rather than silently failing to npm.
Conclusion:
- Difficult to detect the binary is missing versus a standard error with the current library
- If the binary is missing in that scenario, we shouldn't fallback to
npm
because it could silently break the build due to using non-pinned versions - Users should explicitly state they want to abandon the lock using
--no-yarn
to avoid the aforementioned issues, this allows for the fail-over you described, but not automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, to further remark, this was implemented in 17091a0 but removed because of these concerns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Difficult to detect the binary is missing versus a standard error with the current library
You can easily check for ENOENT
If the binary is missing in that scenario, we shouldn't fallback to npm because it could silently break the build due to using non-pinned versions
Did you know that a lockfile does nothing in a dependency? What I mean is that if I install (nom or yarn) execa
which has a yarn.lock
file, the lockfile is not taken into account for the dependencies of execa
. So actually the only thing a lock file does is ensuring the same dependencies between contributors, not between contributors and its users. So falling back to npm
is not a bad thing at all because you would end up with the exact same dependencies as your users! This is an important thing to keep in mind.
So yes, I still think we should fall back to npm if yarn does not exist. You can easily set a custom message in Listr to notify the users that yarn is not installed and that it's falling back to nom because of that. We can even recommend the users that they should install yarn, but we should not be the one that should enforce yarn upon our users. If they don't want it they don't want it, their choice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can easily check for ENOENT
If I have a dependency I cannot install, for instance a OSX only module on the Windows platform, it also triggers a fail-over. execa gives an ENOENT code whether the command could not be found or the installation failed (on Windows at least).
To your second point, yes, I am well aware how a yarn file works. My main issue was it can affect the local directory (and even np tests themselves). A user could be unaware that it defaulted to npm or doesn't understand the consequences of such. I know this type of issue wouldn't pop up often, but it is still a very real issue that could lead to a hard to diagnose problem.
package.json
Outdated
"inquirer": "^1.2.1", | ||
"listr": "^0.6.1", | ||
"listr": "^0.10.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sindresorhus Because we skip quite some versions here, we should test thoroughly first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Master is currently on ^0.9.0
so this shouldn't be a problem, this PR just has an old base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can easily rebase with git rebase master
. We cannot merge automatically now because of conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, ping @gillchristian - or better yet, could you just give me write access to that repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonathandelgado sorry once again 😬
I just gave you push access to my fork
Alright, just squashed history and rebased, thanks for the write access @gillchristian!
Going to stick with my previous comments on falling back to NPM. Hopefully this will be the final review. |
title: 'Installing dependencies', | ||
title: 'Installing dependencies using Yarn', | ||
enabled: () => opts.yarn === true, | ||
task: () => exec('yarn', ['install']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just yarn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They both work, this way is more clear on the intent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh okay, didn't event know it worked :p. I always type yarn
because it's faster. So fine for me for using the more verbose one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Late to the party, but I'm wondering if it makes sense to add the --pure-lockfile
flag since rimraf'ing the node_modules
folder and doing a yarn install
can generate a different lockfile and will fail the "Bumping version" step because the Git working directory is not clean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doing a yarn install can generate a different lockfile
I don't think it is, that's the whole idea of the lock file... Unless you run yarn upgrade
. Or am I wrong? I never experienced a changed yarn.lock file after running yarn
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can happen when a dependency gets upgraded (using NPM perhaps) but the lockfile wasn't regenerated.
I'm not quite sure what the best course of action would be, maybe failing isn't so bad in hindsight 🤔
Great work @gillchristian and @jonathandelgado 🎉 Would either or both of you be interested in being a maintainer on this project? Sam and I could use some help triaging and fixing issues and other things. No worries if not though :) |
One sidenote, we don't accept every feature request as we don't want to bloat this module with too many features. It's already quite feature rich at the moment. But any help would be more then welcome :). |
Yes I would like to help, I cannot promise to be available all the time but I'll do my best 😄 |
Yarn will regenerate the Yarn lock file if the package.json has differences in dependencies since the lock file was last generated. Ie If package.json dependencies unchanged since last yarn.lock generation = use yarn lock exactly, do not change it Else all bets are off: regenerate yarn lock file based on current contents of package.json In my opinion |
@gillchristian Cool! I've invited you to the repo. If you're not sure what to do now, going through the open issues and commenting your thoughts would be a good start. |
This PR adds
yarn
as the default installer, and will fallback tonpm
whenyarn
is not installed.Also adds an
opt-out
flag (--no-yarn
) to override the behavior.Since yarn's
preinstall
andpostinstall
issues are solved I believe is safe to add this option.Fixes #98