Fix for issue #43. install peer deps doesn't update a peer dep when a… #46
Conversation
…r dep when a more recent version is already installed.
| * Read package file file from disk. | ||
| * Returns undefined if no package file exists. | ||
| */ | ||
| function readPackageFile() { |
There was a problem hiding this comment.
Could you add a test for this function in install-peerdeps.test.js under the getPackageData test?
There was a problem hiding this comment.
Thanks, I'll take a look!
| } | ||
| }); | ||
| } else { | ||
| resolve(); |
There was a problem hiding this comment.
My understanding of what occurs in this line is that if the package file does not exist, readPackageFile resolves to undefined. Is this the desired behavior?
There was a problem hiding this comment.
Yes, that's because I'm not assuming there is a package file.
It's possible there isn't one in the current directory.
There was a problem hiding this comment.
Of course, just checking as I wasn't sure if you wanted to reject with an error or resolve with false instead -- regardless I see you're checking for truthiness below this line anyways, so undefined works for me.
| } | ||
| readPackageFile() | ||
| .then(localPackageFile => | ||
| getPackageData({ encodedPackageName, registry, auth, proxy }) |
There was a problem hiding this comment.
One way we can flatten this Promise tree is something like this:
let localPackageFile;
readPackageFile()
.then(l => {
localPackageFile = l;
return getPackageData({ encodedPackageName, registry, auth, proxy });
})
.then(data => {
// ...
// body of function
// ...
})
.catch(err => cb(err));I think it'll make the function a bit easier to read/reason about!
There was a problem hiding this comment.
Personally I'm not keen on setting variables outside the promise callbacks.
Normally these days I use async/await instead of raw promises. You should consider using async/await if you want it easier to read and reason about ;)
There was a problem hiding this comment.
I agree -- async/await is a beautiful abstraction over callbacks and Promises!
Unfortunately, there's a project at Airbnb which depends on this project maintaining compatibility with Node.js 0.10 and 0.12, and I don't want to use async/await yet as I'm not completely sure if regeneratorRuntime has dropped support for earlier Node versions -- so that's why we're staying with Promises for now.
While potentially awkward, setting variables outside of the promise callback is the best way I know of to keep the line depth small (avoiding the pyramid of doom!). If you can think of another way to use promises without nesting the inner promise more deeply while still keeping the localPackageFile variable in scope, though, I welcome that too.
| .then(() => { | ||
| const cli = spawnCli("data-forge-indicators", "has-newer-peer-dep"); | ||
| cli.on("exit", code => { | ||
| t.equal(code, 0, `errored, exit code was ${code}`); |
There was a problem hiding this comment.
This might help with the tests in general -- if you add a third argument like
t.equal(/\byarn global\b/.test(cmd), true, `errored, got ${cmd}`);to the failing tests, you'll be able to see what's going on.
There was a problem hiding this comment.
I'm not sure what you mean.
Where does the cmd variable come from?
There was a problem hiding this comment.
Forgot to mention, but it's from another test a few lines below:
test("places `global` as first arg following `yarn` when using yarn and `--global` is specified", t => {
getCliInstallCommand(["eslint-config-airbnb", "--global", "-Y"]).then(
command => {
const cmd = command.toString();
t.comment(cmd);
t.equal(/\byarn global\b/.test(cmd), true);
t.end();
},
t.fail
);
});
|
@ashleydavis thank you for reading over my comments and taking the time to respond, and thanks for the new test. I hope my responses are helpful and I'm happy to provide more clarification too. |
… more recent version is already installed.