Auto install bundle #9490

redmunds · 2014-10-08T15:43:37Z

Automatically install extensions "bundled" with installer on startup. This feature relies on the installer putting zip files for "bundled" extensions into the [install-dir]/auto-install-extensions/ folder.

This is for #9233

Testing Matrix for `auto-install-extensions` folder

Should fail silently

is missing
is empty
contains file which has non-zip file extension
contains file which has non-extensions with zip file extension
contains extension that is already auto-installed
contains extension that is already manually installed
contains extension that has a newer version auto-installed
contains extension that has a newer version manually installed
contains extension that was already auto-installed, then manually removed

Should install extension

contains extension that is not yet installed (auto or manually)
contains extension that has an older version auto-installed
contains extension that has an older version manually installed

peterflynn · 2014-10-08T18:22:11Z

I haven't looked at the code yet, but regarding those bullets:

The best time to run it might be right after ExtensionLoader.init(), when all the other extensions have loaded. Waiting until appReady seems unnecessary (and slower).
I think we really do still want it to be silent.

redmunds · 2014-10-09T21:20:20Z

Ready for another review:

Installing extensions is working. Actually only tested a single extension. I haven't yet tested updating or extension already installed scenarios.

Code still executes in appReady() since Package module does not start to load extensionsManager node connection until appReady() anyway. And still need to wait until Package._getNodeConnectionDeferred() resolves.
I guess it doesn't need to use appReady() in brackets.js
Code looks for .zip files in [installation-dir]/bundles/ folder -- better name or location?

cc @dangoor @peterflynn

le717 · 2014-10-11T21:01:11Z

src/brackets.js

-            AppInit.appReady(function () {
+        AppInit.appReady(function () {
+            // Check for updates
+            if (!params.get("skipUpdateCheck") && !brackets.inBrowser) {


Since an (eventual) in-browser version can't be updated client-side, wouldn't it be more performant to check for that first?

Yes, that does make sense. That code was not added for this feature, but I went ahead changed it.

I noticed this code was already present, but I don't ever hurts to perform trivial optimizations like this one. :)

…bundle

marcelgerber · 2014-10-13T16:34:14Z

src/extensibility/ExtensionManager.js

+        FileUtils               = require("file/FileUtils"),
+        Strings                 = require("strings"),
+        StringUtils             = require("utils/StringUtils"),
+        ThemeManager            = require("view/ThemeManager");


You don't need that extra indent level.

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

le717 · 2014-10-13T18:59:32Z

src/extensibility/ExtensionManager.js

+            bundles = [],
+            installZips = [],
+            updateZips = [],
+            deferred = new $.Deferred();


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 = [];

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

Done, but I indented everything to same level.

dangoor · 2014-10-14T16:53:59Z

src/extensibility/ExtensionManager.js

+        // Get list of extension bundles
+        var validatePromise,
+            srcPath = FileUtils.getNativeBracketsDirectoryPath(),
+            dirPath = srcPath.substr(0, srcPath.lastIndexOf("/")) + "/bundles/",


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

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...

@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).

@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.

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.

@dangoor Ah, I missed that point. Done.

ingorichter · 2014-10-14T17:13:46Z

I think we should add a couple of tests for this new feature.

ingorichter · 2014-10-14T17:18:42Z

So, the folder will be named bundles? I need to update the E4B build job.

dangoor · 2014-10-14T18:06:13Z

Tests would be good, but I'm concerned with the difficulty in writing tests for this particular bit of code. _autoInstallBundles could likely be broken up in a way that allows testing of the logic independent of integration with the rest of the system.

redmunds · 2014-10-14T20:01:35Z

@ingorichter The latest idea is to call the folder "auto-install-extensions". Any other suggestions?

marcelgerber · 2014-10-14T20:02:07Z

Maybe "preinstalled-extensions" (or even "bundled-extensions")?

redmunds · 2014-10-14T20:59:55Z

@marcelgerber Thanks for the suggestions. This code is doing the installing, so "preinstalled" doesn't sound right to me. I think "auto-installed-extensions" is a little more descriptive than "bundled-extensions".

redmunds · 2014-10-14T21:01:14Z

Ready for another review. All comments have been addressed except tests -- let me know if you have any thoughts about what could and should be tested.

ingorichter · 2014-10-14T23:34:13Z

auto-installed-extensions feels more appropriate to me.

redmunds · 2014-10-15T00:02:32Z

Oops, the actual folder name is slightly different: auto-install-extensions.

dangoor · 2014-10-15T13:53:28Z

It looks like this does not yet handle the final two points in the story:

I modified the installed package.json of an autoinstalled extension to have a higher version number. The extension was re-installed on launch.
I removed the autoinstalled extension via the Extension Manager and it was reinstalled on launch.

redmunds · 2014-10-15T20:39:25Z

@dangoor Ready for another review

redmunds · 2014-10-15T20:43:30Z

I added a testing matrix to description. Let me know when this is ready and I'll squash the commits.

dangoor · 2014-10-16T13:53:28Z

src/extensibility/ExtensionManager.js

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

    // Public exports
+    exports.autoInstallBundles      = autoInstallBundles;


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

@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.

That would be fine, too.

dangoor · 2014-10-16T15:02:42Z

Mark autoInstallBundles private and I think this is ready to go into testing.

A couple of comments about unit testing, because I think that quick to run tests are a great way to specify the details of the expected behavior.

If we had versions of our Async functions that would return the results/rejections in arrays, we could gather up all of the package info and then run that through a nice, easy-to-test, synchronous, pure function that contains all of the logic around checking versions, which ones need install, which ones need update and just returns the install/update instructions. I think that's generally a nicer factoring anyhow (gather up the data, work through the data, process side effects).

Alternatively, if we had a good way to mock modules (ideally like rewire), we could test the whole routine by mocking out just the bits we need. I think it should be possible to do what rewire does in our test context and I'll play with that sometime.

At the moment, we have neither of those infrastructure pieces to do that testing...

redmunds · 2014-10-16T16:43:15Z

This PR is replaced by #9573. Closing.

redmunds added 3 commits October 7, 2014 18:45

auto-install bundles

2c75710

process zip files

f4b1dd0

cleanup

be1fcd6

wait until extensionManage node connection starts; install silently

d2b3163

remove unused var

c758209

redmunds changed the title ~~[Review] Auto install bundle~~ Auto install bundle Oct 10, 2014

le717 reviewed Oct 11, 2014
View reviewed changes

redmunds added 2 commits October 13, 2014 08:52

Merge remote-tracking branch 'origin/master' into randy/auto-install-…

a4d4c58

…bundle

check local values first

7881005

marcelgerber reviewed Oct 13, 2014
View reviewed changes

changes for code review

7d311b9

le717 reviewed Oct 13, 2014
View reviewed changes

dangoor self-assigned this Oct 14, 2014

dangoor added the PR Triage Complete label Oct 14, 2014

dangoor reviewed Oct 14, 2014
View reviewed changes

changes for code review

4af80b1

keep track of auto-install extensions; compare version numbers

89550d7

cleanup

1e6b6b0

dangoor reviewed Oct 16, 2014
View reviewed changes

redmunds added 2 commits October 16, 2014 09:34

no need for public api

3341176

cleanup

8ec89c8

redmunds mentioned this pull request Oct 16, 2014

Auto-install extensions #9573

Merged

redmunds closed this Oct 18, 2014

le717 mentioned this pull request Oct 21, 2014

Remove deprecated file system APIs #9622

Merged

redmunds deleted the randy/auto-install-bundle branch December 5, 2014 19:02

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Auto install bundle brackets-archive/bracketsIssues#8478

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto install bundle #9490

Auto install bundle #9490

redmunds commented Oct 8, 2014

peterflynn commented Oct 8, 2014

redmunds commented Oct 9, 2014

le717 Oct 11, 2014

redmunds Oct 13, 2014

le717 Oct 13, 2014

marcelgerber Oct 13, 2014

redmunds Oct 13, 2014

le717 Oct 13, 2014

dangoor Oct 14, 2014

redmunds Oct 14, 2014

dangoor Oct 14, 2014

peterflynn Oct 14, 2014

redmunds Oct 14, 2014

redmunds Oct 14, 2014

dangoor Oct 14, 2014

redmunds Oct 14, 2014

ingorichter commented Oct 14, 2014

ingorichter commented Oct 14, 2014

dangoor commented Oct 14, 2014

redmunds commented Oct 14, 2014

marcelgerber commented Oct 14, 2014

redmunds commented Oct 14, 2014

redmunds commented Oct 14, 2014

ingorichter commented Oct 14, 2014

redmunds commented Oct 15, 2014

dangoor commented Oct 15, 2014

redmunds commented Oct 15, 2014

redmunds commented Oct 15, 2014

dangoor Oct 16, 2014

redmunds Oct 16, 2014

dangoor Oct 16, 2014

dangoor commented Oct 16, 2014

redmunds commented Oct 16, 2014

Auto install bundle #9490

Auto install bundle #9490

Conversation

redmunds commented Oct 8, 2014

Testing Matrix for auto-install-extensions folder

Should fail silently

Should install extension

peterflynn commented Oct 8, 2014

redmunds commented Oct 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ingorichter commented Oct 14, 2014

ingorichter commented Oct 14, 2014

dangoor commented Oct 14, 2014

redmunds commented Oct 14, 2014

marcelgerber commented Oct 14, 2014

redmunds commented Oct 14, 2014

redmunds commented Oct 14, 2014

ingorichter commented Oct 14, 2014

redmunds commented Oct 15, 2014

dangoor commented Oct 15, 2014

redmunds commented Oct 15, 2014

redmunds commented Oct 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dangoor commented Oct 16, 2014

redmunds commented Oct 16, 2014

Testing Matrix for `auto-install-extensions` folder