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

Auto install bundle #9490

Closed
wants to merge 13 commits into from
Closed

Auto install bundle #9490

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
2c75710
auto-install bundles
redmunds Oct 8, 2014
f4b1dd0
process zip files
redmunds Oct 8, 2014
be1fcd6
cleanup
redmunds Oct 8, 2014
d2b3163
wait until extensionManage node connection starts; install silently
redmunds Oct 9, 2014
c758209
remove unused var
redmunds Oct 10, 2014
a4d4c58
Merge remote-tracking branch 'origin/master' into randy/auto-install-…
redmunds Oct 13, 2014
7881005
check local values first
redmunds Oct 13, 2014
7d311b9
changes for code review
redmunds Oct 13, 2014
4af80b1
changes for code review
redmunds Oct 14, 2014
89550d7
keep track of auto-install extensions; compare version numbers
redmunds Oct 15, 2014
1e6b6b0
cleanup
redmunds Oct 15, 2014
3341176
no need for public api
redmunds Oct 16, 2014
8ec89c8
cleanup
redmunds Oct 16, 2014
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
13 changes: 8 additions & 5 deletions src/brackets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ define(function (require, exports, module) {
UpdateNotification = require("utils/UpdateNotification"),
UrlParams = require("utils/UrlParams").UrlParams,
PreferencesManager = require("preferences/PreferencesManager"),
ExtensionManager = require("extensibility/ExtensionManager"),
ExtensionUtils = require("utils/ExtensionUtils"),
DragAndDrop = require("utils/DragAndDrop"),
CodeInspection = require("language/CodeInspection"),
Expand Down Expand Up @@ -315,13 +316,15 @@ define(function (require, exports, module) {
});
});

// Check for updates
if (!params.get("skipUpdateCheck") && !brackets.inBrowser) {
AppInit.appReady(function () {
AppInit.appReady(function () {
// Check for updates
if (!brackets.inBrowser && !params.get("skipUpdateCheck")) {
// launches periodic checks for updates cca every 24 hours
UpdateNotification.launchAutomaticUpdate();
});
}
}

ExtensionManager.autoInstallBundles();
});
}

/**
Expand Down
106 changes: 97 additions & 9 deletions src/extensibility/ExtensionManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,16 @@
define(function (require, exports, module) {
"use strict";

var _ = require("thirdparty/lodash"),
Package = require("extensibility/Package"),
Async = require("utils/Async"),
ExtensionLoader = require("utils/ExtensionLoader"),
ExtensionUtils = require("utils/ExtensionUtils"),
FileSystem = require("filesystem/FileSystem"),
Strings = require("strings"),
StringUtils = require("utils/StringUtils"),
ThemeManager = require("view/ThemeManager");
var _ = require("thirdparty/lodash"),
Package = require("extensibility/Package"),
Async = require("utils/Async"),
ExtensionLoader = require("utils/ExtensionLoader"),
ExtensionUtils = require("utils/ExtensionUtils"),
FileSystem = require("filesystem/FileSystem"),
FileUtils = require("file/FileUtils"),
Strings = require("strings"),
StringUtils = require("utils/StringUtils"),
ThemeManager = require("view/ThemeManager");
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need that extra indent level.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I removed the dependency that I increased the indentation for :)


// semver.browser is an AMD-compatible module
var semver = require("extensibility/node/node_modules/semver/semver.browser");
Expand Down Expand Up @@ -600,12 +601,99 @@ define(function (require, exports, module) {
}, []);
}

function _autoInstallBundles() {
// Get list of extension bundles
var validatePromise,
srcPath = FileUtils.getNativeBracketsDirectoryPath(),
dirPath = srcPath.substr(0, srcPath.lastIndexOf("/")) + "/bundles/",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: might be nicer to say `dirPath = FileUtils.getDirectoryPath(FileUtils.getNativeBracketsDirectoryPath()) + "bundles/";

Copy link
Member

Choose a reason for hiding this comment

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

For the folder name, something a little more self-explanatory might be good... e.g. auto-install-extensions. ("bundle" is a term we haven't really used for extensions before, anyway).

Also might be good to move the folder name into a constant...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dangoor It depends on where we want to put the folder. FileUtils.getDirectoryPath(FileUtils.getNativeBracketsDirectoryPath()) returns [install-path]/www so your suggestion would put new folder inside the /www folder (i.e. [install-path]/www/bundles). But, I wanted it to be a sibling to /www (i.e. [install-path]/bundles).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peterflynn I started with "bundles" just to get the code going, but I agree that it's not quite right. To me, the "bundle" is the "installer plus the extensions" (not just the extensions). Unless I hear any better suggestions, I'll change it to "auto-install-extensions" and put it in a constant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not so. FileUtils.getDirectoryPath does basically what your code does: it chops off until the last slash. Since getNativeBracketsDirectoryPath is old-style and doesn't include the trailing slash, the result doesn't go into www.

> FileUtils.getDirectoryPath(FileUtils.getNativeBracketsDirectoryPath())
"/Applications/Brackets.app/Contents/dev/"

I agree with Peter's points also about auto-install-extensions and using a constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dangoor Ah, I missed that point. Done.

bundles = [],
installZips = [],
updateZips = [],
deferred = new $.Deferred();
Copy link
Contributor

Choose a reason for hiding this comment

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

Code style: this would look a bit better like so:

var validatePromise,
    srcPath  = FileUtils.getNativeBracketsDirectoryPath(),
    dirPath  = srcPath.substr(0, srcPath.lastIndexOf("/")) + "/bundles/",
    deferred = new $.Deferred(),
    bundles     = [],
    updateZips  = [],
    installZips = [];

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, I find the "Align It!" extension really handy for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, but I indented everything to same level.


FileSystem.getDirectoryForPath(dirPath).getContents(function (err, contents) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the "bundles" folder doesn't exist?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe getContents would be called with an err.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FileSystem.getDirectoryForPath(dirPath) will return an EntryConstructor object, then "what @dangoor said" :)

if (!err) {
var i, dirItem;

for (i = 0; i < contents.length; i++) {
dirItem = contents[i];
if (dirItem.isFile && FileUtils.getFileExtension(dirItem.fullPath) === "zip") {
bundles.push(dirItem);
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use Array.filter here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks -- much cleaner. Looking at this code also made me realize that the callback from getContents() is async, so I fixed the code for that.

}
}
}
});

// Process bundles
// TODO - currently we're looking for extension .zip files in "bundles" folder
// - not really "bundles", right? Just need better terminology.

// Parse zip files and separate new installs vs. updates
validatePromise = Async.doInParallel(bundles, function (file) {
var result = new $.Deferred();

// Call validate() so that we open the local zip file and parse the
// package.json. We need the name to detect if this zip will be a
// new install or an update.
Package.validate(file.fullPath, { requirePackageJSON: true }).done(function (info) {
if (info.errors.length) {
result.reject(Package.formatError(info.errors));
return;
}

var extensionName = info.metadata.name,
extensionInfo = extensions[extensionName],
isUpdate = extensionInfo && !!extensionInfo.installInfo;

if (isUpdate) {
updateZips.push(file);
} else {
installZips.push(file);
}

result.resolve();
}).fail(function (err) {
result.reject(Package.formatError(err));
});

return result.promise();
});


validatePromise.done(function () {
var installPromise = Async.doSequentially(installZips, function (file) {
return Package.installFromPath(file.fullPath);
});

var updatePromise = installPromise.always(function () {
return Async.doSequentially(updateZips, function (file) {
return Package.installUpdate(file.fullPath);
});
});

// Always resolve the outer promise
updatePromise.always(deferred.resolve);
}).fail(function (errorArray) {
deferred.reject(errorArray);
});

return deferred.promise();
}

function autoInstallBundles() {
// TODO: make _getNodeConnectionDeferred() public?
Package._getNodeConnectionDeferred().done(function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't looked at Package recently, but is this even required? It seems like the node connection should be an implementation detail of Package that the caller need not be aware of. (In other words, you can call validate and it will return after it has a connection and is ready to return a value.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without waiting on this promise to finish, I get: "extensionManager domain is undefined" and everything fails. Let me know if you can think of a cleaner way to do this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah... it looks like Package easily could wait until Node is ready, but it doesn't. All the calls are wrapped through Package._extensionManagerCall(), but it just rejects with an error if Node isn't ready yet, instead of waiting. Weird. I guess Extension Manager itself relies on the fact that it's impossible for users to click fast enough to get into this code before Node has finished booting up.

_autoInstallBundles();
});
}

// Listen to extension load and loadFailed events
$(ExtensionLoader)
.on("load", _handleExtensionLoad)
.on("loadFailed", _handleExtensionLoad);

// Public exports
exports.autoInstallBundles = autoInstallBundles;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should call this private API. The only caller of this should be Brackets itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dangoor What if ExtensionManager listens for AppInit.appReady() (instead of calling it from brackets.js) ? This way it does not need to be exported at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would be fine, too.

exports.downloadRegistry = downloadRegistry;
exports.getCompatibilityInfo = getCompatibilityInfo;
exports.getExtensionURL = getExtensionURL;
Expand Down