Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

fixes #11552 #11553

Merged
merged 2 commits into from
Sep 11, 2015
Merged

fixes #11552 #11553

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
887a77f
fixes #11552
zaggino Aug 6, 2015
6afab58
add pCallback param to docs
zaggino Aug 7, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions src/extensibility/node/ExtensionManagerDomain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,9 +205,10 @@ function legacyPackageCheck(legacyDirectory) {
* @param {{disabledDirectory: !string, apiVersion: !string, nameHint: ?string,
* systemExtensionDirectory: !string}} additional settings to control the installation
* @param {function} callback (err, result)
* @param {function} pCallback (msg) callback for notifications about operation progress
* @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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is usually better to add new params at the end, unless there is a reason.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a reason, progress callback was added right after callback argument, unfortunately, it also caused a collision with this "private" argument

if (!options || !options.disabledDirectory || !options.apiVersion || !options.systemExtensionDirectory) {
callback(new Error(Errors.MISSING_REQUIRED_OPTIONS), null);
return;
Expand Down Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this actually make a difference?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :-)

if (hasLegacyPackage) {
// When there's a legacy installed extension, remove it first,
// then also remove any new-style directory the user may have.
Expand Down Expand Up @@ -333,9 +334,10 @@ function _cmdInstall(packagePath, destinationDirectory, options, callback, _doUp
* @param {{disabledDirectory: !string, apiVersion: !string, nameHint: ?string,
* systemExtensionDirectory: !string}} additional settings to control the installation
* @param {function} callback (err, result)
* @param {function} pCallback (msg) callback for notifications about operation progress
*/
function _cmdUpdate(packagePath, destinationDirectory, options, callback) {
_cmdInstall(packagePath, destinationDirectory, options, callback, true);
function _cmdUpdate(packagePath, destinationDirectory, options, callback, pCallback) {
_cmdInstall(packagePath, destinationDirectory, options, callback, pCallback, true);
}

/**
Expand Down Expand Up @@ -375,7 +377,7 @@ function _endDownload(downloadId, error) {
/**
* Implements "downloadFile" command, asynchronously.
*/
function _cmdDownloadFile(downloadId, url, proxy, callback) {
function _cmdDownloadFile(downloadId, url, proxy, callback, pCallback) {
// Backwards compatibility check, added in 0.37
if (typeof proxy === "function") {
callback = proxy;
Expand Down Expand Up @@ -436,7 +438,7 @@ function _cmdAbortDownload(downloadId) {
/**
* Implements the remove extension command.
*/
function _cmdRemove(extensionDir, callback) {
function _cmdRemove(extensionDir, callback, pCallback) {
fs.remove(extensionDir, function (err) {
if (err) {
callback(err);
Expand Down