[redmunds]

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]

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]

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]

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

[redmunds]

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

[le717]

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

[marcelgerber]

You don't need that extra indent level.

[redmunds]

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

[le717]

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

[dangoor]

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

[redmunds]

Done, but I indented everything to same level.

[dangoor]

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

[peterflynn]

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

[redmunds]

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

[redmunds]

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

[dangoor]

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.

[redmunds]

@dangoor Ah, I missed that point. Done.

[ingorichter]

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

[ingorichter]

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

[dangoor]

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]

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

[marcelgerber]

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

[redmunds]

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

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]

auto-installed-extensions feels more appropriate to me.

[redmunds]

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

[dangoor]

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]

@dangoor Ready for another review

[redmunds]

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

[dangoor]

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

[redmunds]

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

[dangoor]

That would be fine, too.

[dangoor]

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]

This PR is replaced by #9573. Closing.