Validate and install extensions (final) #3158

njx · 2013-03-18T23:47:53Z

This supersedes #3137, and should contain just the changes for the installer pull request separate from the node modules in #3156.

existence of package.json

missing name and version metadata

…autodismiss

…e basic unit tests

… keyboard handling in various states.

…empty again

…ate, but if you can access the class show() should be public). Also, remove the installer param to _showDialog() since it's never used.

installed (installing into disabled in those cases)

legally, and the test that follows the deleted one catches validation issues in general)

the installation and mocks the extension loading to verify that the extension *would* load. disabled the test because it can only run once and will fail on the second run because there's no way to delete the target directory

(from GitHub for example)

njx · 2013-03-19T02:14:21Z

src/extensibility/Package.js

+            .done(function (localPath) {
+                state = STATE_INSTALLING;
+
+                // TOOD: need to clean up ZIP from temp location


Looks like this comment is stale now since we call unlink() after the installation succeeds or fails below.

Yep, sorry. Removed.

njx · 2013-03-19T02:38:13Z

@dangoor @peterflynn -- re-review (for non-UI parts) complete. I'm not going to merge right now, but will be online later tonight--Peter, if you push some fixes up before I get back online, that's cool, otherwise I'll likely go ahead and merge then; there aren't really any major showstoppers that I see, though there are a few things we should definitely address before DoD.

@dangoor -- don't remember where I mentioned this before, but the main changes for the UI stuff (outside of InstallExtensionDialog) were:

refactor out the CSS for busy spinner so we use common code everywhere
slightly refactor the HTML/CSS for the Project Settings dialog so I can use the same structure in the Install Extension dialog
fix the button positioning in a couple of dialogs that had Cancel on the left side of the dialog instead of the right (it should be on the right, but to the left of the OK button)
add an autoDismiss flag to showModalDialogUsingTemplate() that allows the caller to manually handle button clicks when set to false (instead of the dialog automatically dismissing itself)

pthiess · 2013-03-19T02:45:41Z

@njx @dangoor @peterflynn Thank you guys for your hard work on this exciting pull into Brackets. Very cool!

Validate and install extensions (final)

njx · 2013-03-19T05:52:01Z

Went ahead and merged this. @dangoor -- if you end up having any comments on the UI stuff, feel free to add the comments to this pull request and I can look at them here later.

dangoor · 2013-03-19T12:57:31Z

src/extensibility/InstallExtensionDialog.js

+                .fail(function (err) {
+                    // Ignore expected case: the Promise is failed when we programmatically cancel
+                    // In all other cases, record the error and transition to error display UI
+                    if (!(err === "CANCELED" && self._state === STATE_INSTALL_CANCELLED)) {


This is an almost embarrassing nit to bring up... there are two different spellings of CANCELED on the same line. Both are legit but canceled (one L) is the more common American way. Someone could come along later and misspell STATE_INSTALL_CANCELLED as a result, but I should hope the error would be obvious and we have JS autocompletion to help anyhow.

Funny, I always preferred "cancelled", but it does look like American English is standardizing on one-l, and we already have "CANCELED" elsewhere in the API (e.g. dialogs). I've fixed this locally.

Actually Peter is fixing it :)

I've fixed this in the process of doing all the async-cancellation code.

dangoor · 2013-03-19T14:30:56Z

I've finished reviewing the UI code.

dangoor · 2013-03-19T19:07:19Z

src/extensibility/node/ExtensionManagerDomain.js

+ */
+function _cmdDownloadFile(downloadId, url, localPath, callback) {
+    if (pendingDownloads[downloadId]) {
+        callback(Errors.DOWNLOAD_ID_IN_USE, null);


I had commented in email separately, but I thought I should put it here with the other comments as well... it appears that the convention in Node is to pass Error objects, so this (and the callback below) should really be something like

callback(new Error(Errors.DOWNLOAD_ID_IN_USE), null);

@dangoor it seems like this is more analogous to the validation failures than it is to throwing an exception in synchronous code... so perhaps I should pass a success object containing an error(s) property instead? Is the first arg of the callback basically used only in cases where the code has essentially 'crashed' or hit an internal error?

It also looks like our node_core code fail to JSONify Error objects properly... we'll need to fix that too.

And actually of course, the standard Error.message doesn't support passing back structured data like the arrays we use for formatted errors in this file. So it seems like we have three options:

Use the success argument with an 'error(s)' property for most problems -- anything other than unexpected, thrown exceptions. (Seems a little gross and not really how most Node treats anticipated error cases).

Use a custom Error subclass that accepts a (non-string) data parameter, and add special logic to ConnectionManager to serialize back that parameter. (Doesn't seem that common in Node either, but still a bit cleaner than above).

Pass something other than an Error as the first arg (some references say this is ok). (But definitely not the standard Node way).

Right now our code seems evenly split between the 1st & 3rd approaches, which is not that ideal...

Also, @dangoor I think the existing change from strings to Errors is causing #3174 since the client side isn't getting any sort of error code info back anymore

Where I started with this was that for validation I assumed that unless the validation routine was actually broken, the whole point was to return whether or not the extension was valid. I carried that over into installation because installation validates first.

I do think you're right that the Node convention is generally to pass (err, null) for anything that goes wrong.

I did see one page about errors that talked about doing this:

var err = new Error("message"); err.data = {more: "stuff"};

It would certainly be more pleasant to have a constructor that can take additional data. And I do think that ConnectionManager should know what to do if the callback is given an error, since that's so common. (As some point, we should have a domain (in Node terms) to capture the errors and carry them up to the UI.)

I don't know how much work there would be in ConnectionManager, but I do think it's a good idea to stick to the node convention where possible.

Do you think validation should return (err, null) if the extension is invalid and (null, metadata) if it's valid?

peterflynn · 2013-03-20T00:40:52Z

src/extensibility/node/ExtensionManagerDomain.js

+                if (!options || !options.disabledDirectory) {
+                    callback(new Error(Errors.NO_DISABLED_DIRECTORY), null);
+                    return;
+                }


@dangoor I'm wondering if we should just make disabledDirectory required... It seems like if one of the standard codepaths will always throw an error without it, it'd be nicer just to make it a required parameter and dispense with the two copies of this code that guards against it being missing.

It seems like apiVersion shouldn't be optional either... unless we're trying to generalize this code to run in some app other than Brackets, it seems safe to assume that install() can always get an API version for context. (Not true of validate() when it runs on the server, of course, but install() seems ok to assume it's running against an actual app).

Sure, I'm fine with making those required (and will do so)

dangoor and others added 30 commits March 18, 2013 16:44

initial package validation. includes metadata and checks for

827916a

existence of package.json

move package.json into node directory (which makes more sense)

8559e71

error messages from package validator are now localized

383a86c

now handles error conditions, invalid zip files, invalid json and

ceef1b5

missing name and version metadata

removed redundancy in the test code

4ec00a2

Added flag to showModalDialogUsingTemplate() to allow dialogs to not …

4d7bf63

…autodismiss

Initial implementation of Install dialog, with a mock backend and som…

ec0b0ae

…e basic unit tests

added comments

dd73be8

checks for a semver compatible version number

b445f8b

add invalid version number to strings

9592314

update test to reflect updated version number in basic-valid-extension

2a6abc3

adds check for main.js

7b13bc0

Moved installation dialog into extensibility folder

8043fb2

ignore node_modules everywhere for jshint

8d118c6

Disable install button while url field is empty. Added unit tests for…

047b054

… keyboard handling in various states.

Converted install extension dialog from singleton to a class

5f0a531

Added unit test for re-disabling Install button if url field becomes …

d2e365e

…empty again

Make InstallerExtensionDialog.show() public (the class itself is priv…

9078146

…ate, but if you can access the class show() should be public). Also, remove the installer param to _showDialog() since it's never used.

handles basic installation

a84d9ed

installation handles the case of extensions that are already

0902092

installed (installing into disabled in those cases)

removes the requirement for a package.json

39c271b

wired installation through to the client

da69600

remove test that is no longer relevant (package.json can be missing

7d8fb47

legally, and the test that follows the deleted one catches validation issues in general)

created an automated test for installation that verifies

51d0e90

the installation and mocks the extension loading to verify that the extension *would* load. disabled the test because it can only run once and will fail on the second run because there's no way to delete the target directory

added comments and a string for ALREADY_INSTALLED

3efc683

Ensure that new state is set before processing the state transition

b8c192b

Added unit tests for cases where installer succeeds/fails synchronously

347ea7c

switch to the .spec.js convention used by brackets-jasmine

f416ecb

handles zip files with directories and removes common prefixes

0aaeb45

(from GitHub for example)

pause reading the file so that the directory can be created

4a5b1d0

njx reviewed Mar 19, 2013
View reviewed changes

njx added a commit that referenced this pull request Mar 19, 2013

Merge pull request #3158 from adobe/install-extensions-rebased

730c806

Validate and install extensions (final)

njx merged commit 730c806 into install-extensions-modules Mar 19, 2013

njx mentioned this pull request Mar 19, 2013

Handle code review comments from #3158 #3159

Closed

dangoor reviewed Mar 19, 2013
View reviewed changes

peterflynn mentioned this pull request Mar 19, 2013

Nicer handling for GitHub URL common cases #3167

Merged

dangoor reviewed Mar 19, 2013
View reviewed changes

njx mentioned this pull request Mar 19, 2013

UI fixes for installer dialog #3173

Merged

peterflynn reviewed Mar 20, 2013
View reviewed changes

This was referenced Mar 20, 2013

Code review fixes for extension download #3178

Merged

Fix 3174 (unknown internal error when installing non-zip file) #3179

Closed

This was referenced Aug 29, 2021

[CLOSED] Validate and install extensions (final) brackets-archive/bracketsIssues#2968

Open

[CLOSED] Fix 3174 (unknown internal error when installing non-zip file) brackets-archive/bracketsIssues#2989

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate and install extensions (final) #3158

Validate and install extensions (final) #3158

njx commented Mar 18, 2013

njx Mar 19, 2013

peterflynn Mar 19, 2013

njx commented Mar 19, 2013

pthiess commented Mar 19, 2013

njx commented Mar 19, 2013

dangoor Mar 19, 2013

njx Mar 19, 2013

njx Mar 19, 2013

peterflynn Mar 19, 2013

dangoor commented Mar 19, 2013

dangoor Mar 19, 2013

peterflynn Mar 19, 2013

peterflynn Mar 19, 2013

peterflynn Mar 19, 2013

peterflynn Mar 19, 2013

dangoor Mar 20, 2013

peterflynn Mar 20, 2013

dangoor Mar 20, 2013

Validate and install extensions (final) #3158

Validate and install extensions (final) #3158

Conversation

njx commented Mar 18, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njx commented Mar 19, 2013

pthiess commented Mar 19, 2013

njx commented Mar 19, 2013

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

dangoor commented Mar 19, 2013

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