fixes #11552 #11553
fixes #11552 #11553
Conversation
|
@zaggino: just a quick comment. I see that the documentation for the apis is missing for the new parameter. Could you add description for the newly added parameter at all the places where it is added? |
| @@ -207,7 +207,7 @@ function legacyPackageCheck(legacyDirectory) { | |||
| * @param {function} callback (err, result) | |||
| * @param {boolean} _doUpdate private argument to signal that an update should be performed | |||
| */ | |||
| function _cmdInstall(packagePath, destinationDirectory, options, callback, _doUpdate) { | |||
| function _cmdInstall(packagePath, destinationDirectory, options, callback, pCallback, _doUpdate) { | |||
There was a problem hiding this comment.
It is usually better to add new params at the end, unless there is a reason.
There was a problem hiding this comment.
there's a reason, progress callback was added right after callback argument, unfortunately, it also caused a collision with this "private" argument
|
params noted where they were missing |
| @@ -272,7 +273,7 @@ function _cmdInstall(packagePath, destinationDirectory, options, callback, _doUp | |||
| // If the extension is already there, we signal to the front end that it's already installed | |||
| // unless the front end has signaled an intent to update. | |||
| if (hasLegacyPackage || fs.existsSync(installDirectory) || fs.existsSync(systemInstallDirectory)) { | |||
| if (_doUpdate) { | |||
| if (_doUpdate === true) { | |||
There was a problem hiding this comment.
Does this actually make a difference?
There was a problem hiding this comment.
I'm assuming the intent was to prevent similar bugs in the future: if you accidentally pass an extra callback arg in this position, it's truthy but not === true.
That said, silently acting like the param was false doesn't necessarily mean we're preventing the mistake from causing a bug. Might be nice to have something like console.assert(_doUpdate === undefined || typeof _doUpdate === "boolean") at the top if we're truly worried about this problem happening again.
Or the other option would be to add a unit test that covers this, so that regardless of what breaks it we'd find out via a failing test :-)
|
@zaggino Can we check this change in and add the unit tests as another PR or should this pr be updated? |
|
sure, this is good to check-in... and sorry, but i don't have time to put together a proper test right now |
|
thanks 👍 |
for #11552