use npm to download extension dependencies after installing from registry

[zaggino]

Seems to be working fine. Tests included, but currently they are disabled in Gruntfile so they need to be run manually, wasn't investigating what was the reason for disabling the tests.

[le717]

[mackenza]

Is this being considered for merging? I think it's a great addition speaking as some who has published a node-based extension with dependencies.

[zaggino]

@mackenza not sure if this will make its way into Brackets, but I've been playing with a concept of going around Brackets' extension registry altogether and download whole extensions from npm. It works conceptually, but needs more testing and polishing, see here: https://github.com/zaggino/brackets-npm-registry

[mackenza]

ah, I see. That would closely resemble what Atom does with APM wrapping
NPM, yes?

On Tue, Mar 17, 2015 at 5:10 AM, Martin Zagora notifications@github.com
wrote:

@mackenza https://github.com/mackenza not sure if this will make its
way into Brackets, but I've been playing with a concept of going around
Brackets' extension registry altogether and download whole extensions from
npm. It works conceptually, but needs more testing and polishing, see here:
https://github.com/zaggino/brackets-npm-registry

—
Reply to this email directly or view it on GitHub
#10602 (comment).

[zaggino]

To be honest, I've no idea what Atom does or doesn't do, never had time to take a closer look at it.

[mackenza]

Yeah, basically this is what they do. The have the Atom Package Manager
which seems to add some Atom-specific meta data fields to Package.json and
they use NPM as the actual dependency manager.

Atom is a hot mess, honestly. Truly an editor without an identity.

On Tue, Mar 17, 2015 at 6:22 PM, Martin Zagora notifications@github.com
wrote:

To be honest, I've no idea what Atom does or doesn't do, never had time to
take a closer look at it.

—
Reply to this email directly or view it on GitHub
#10602 (comment).

[nethip]

@zaggino Thanks for putting up this PR! Great work and thinking. I like the idea of keeping the node extensions lean and downloading the dependencies using npm.

Couple of things to consider for this PR.

1. What changes do we need to do for our Registry(like changing validation logic e.t.c)?
2. I have some concerns about versioning of the dependent npm modules. If an extension developer mentions the version of the dependent module to be something like >0.1.3, what happens if the latest version of that dependent module is buggy and is not compatible with that extension?
3. What can we do something similar which can help extension developers who do not use node modules?

Talking about the third point, how about downloading the extension itself from just a link? Something like, the registry will only have links (mostly to GitHub repositories) and we download the extension itself from Github repository/external link followed by installing the dependencies(which this PR is about), if any.

CC @ryanstewart @peterflynn

[mackenza]

@nethip I would think point 2 is a concern for any dependency manager and not unique to this PR's application of NPM. If a package author doesn't want to "roll the dice" on having package updates break their extension, they should be explicit about the dependency. i.e. 1.0.3 rather than >=1.0.3

On 3, NPM is quickly becoming the dependency manager to rule them all. Certainly it's not restricted to Node modules anymore. I can find most of the modules I use for front end JS in there, as well. I think what @zaggino is suggesting is we use the current package registry to download the actual extension zip (as we do today) but as part of the installation, rather than just unzipping the files, we also run npm install to pick up the dependencies. This sounds like the best of both worlds.

[zaggino]

@ficristo @petetnt @abose @ingorichter this would be worth a second look, it's no magic and it'd help a lot for extensions so they wouldn't have to bundle everything in a .zip file

[petetnt]

Have you tested this out with npm@3. I didn't have much issues (if any) from moving to 2->3 when 3 started rolling out but I don't know if it has some troubles with how Brackets resolves the extension dependencies.

[zaggino]

No, I could test/fix this to work with 3 but original PR was put together when 2 was latest and I don't think it's worth spending time on it if it won't be merged before npm 4 comes out.

[petetnt]

Fair enough. @3 would bring some nice benefits, but I do see how testing for it might be unfeasible at this point. I'd like to see this land sooner than the later though, preferably with 3 😼

[zaggino]

The review I asked for is more about the idea of this then the code (I'm happy to fix/update it), when I first put up this PR there wasn't enough will to merge this and potentially deal with problems like invalid dependencies in package.json and etc. But I still believe this would help a lot of extension authors to move forward.

[zaggino]

Oh, and npm@3 doesn't play well with node 0.10 so we need to merge two other PRs first.

[petetnt]

Yeah, good catch on that one. I'd love to get rid of both of those ancient dependencies at the same time 👍

[petetnt]

Looks good to me with some minor nits about personal preferences.

[ficristo]

I like the idea but I defer the final decision to others, I'm not confident on how this will interact with the registry. A part the following few questions, LGTM.

I suppose we can start to using dependencies in the root package.json if things aren't extensions, right?
Can you use require instead of JSON.parse(fs.readFileSync...?
The API of npm are really supported? Looking around seems not. Can you use instead an exec command or similar and run npm install --production?
Your npm extension provide a reinstall button, don't we need it?
On VSCode by default they are exluding **/.git/object/** instead of .git/**, aren't your git extension affected? (Probably I was thinking to #12647 while writing this...)

[zaggino]

@petetnt @ficristo updated the PR per your suggestions.
Tested by installing https://github.com/zaggino/brackets-coffeelint from an URL.

[ficristo]

Cannot you use require instead of JSON.parse(fs.readFileSync?

[ficristo]

I didn't expect to become complex like this, but I believe is better than using npm API. Thank you @zaggino.
From my side looks good.

[zaggino]

• rebased as requested by @swmitra
• added proxy support requested by @ficristo
• tested both with and without proxy by installing https://github.com/zaggino/brackets-coffeelint from an URL

note: original bundled request was failing with my proxy but upgrade to the latest version fixed that, so I've added this to my commit even though it was not directly related - request is now managed as npm dependency (it can be in root as it's node-only dependency) and old request committed to the repository was removed

[ficristo]

IMHO if we change extensibility/node in this way we should wait for #12933 first.
(Thank you for the update!)

[zaggino]

#12933 depends on the access to the registry machine and I don't know what else, so it might take a year or two to fix that...

[zaggino]

removed request update which @ficristo complained about, should be ready to merge

[swmitra]

Great work as always @zaggino 👍 .
I guess this PR is good to go now and if we merge now, it will give us some additional time for testing as well.

[swmitra]

Merging now ...

[zaggino]

btw, fun fact: this PR took exactly 2 years to merge

[swmitra]

I bet it's better late than never 👍
I hope we will never see a PR raised today getting merged on 15th Feb 2019 😃

[zaggino]

Tested this by publishing a copy of archive from npm into brackets registry (TypeScript 0.9.4) and it works great. Thanks a lot guys for getting this to the core.