Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CLOSED] use npm to download extension dependencies after installing from registry #9350

Open
core-ai-bot opened this issue Aug 30, 2021 · 30 comments
Open

[CLOSED] use npm to download extension dependencies after installing from registry #9350

core-ai-bot opened this issue Aug 30, 2021 · 30 comments

Comments

@core-ai-bot
Copy link
Member

Issue by zaggino
Sunday Feb 15, 2015 at 02:29 GMT
Originally opened as adobe/brackets#10602

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.

zaggino included the following code: https://github.com/adobe/brackets/pull/10602/commits
@core-ai-bot
Copy link
Member Author

Comment by le717
Sunday Feb 15, 2015 at 03:54 GMT

@core-ai-bot
Copy link
Member Author

Comment by mackenza
Tuesday Mar 17, 2015 at 08:21 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Tuesday Mar 17, 2015 at 09:10 GMT

@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

@core-ai-bot
Copy link
Member Author

Comment by mackenza
Tuesday Mar 17, 2015 at 18:36 GMT

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 [email protected]
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
adobe/brackets#10602 (comment).

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Tuesday Mar 17, 2015 at 22:21 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by mackenza
Tuesday Mar 17, 2015 at 23:57 GMT

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 [email protected]
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
adobe/brackets#10602 (comment).

@core-ai-bot
Copy link
Member Author

Comment by nethip
Wednesday Mar 18, 2015 at 06:27 GMT

@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

@core-ai-bot
Copy link
Member Author

Comment by mackenza
Wednesday Mar 18, 2015 at 15:13 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Tuesday Aug 16, 2016 at 04:26 GMT

@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

@core-ai-bot
Copy link
Member Author

Comment by petetnt
Tuesday Aug 16, 2016 at 06:18 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Tuesday Aug 16, 2016 at 07:12 GMT

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 adobe/brackets#12647 while writing this...)

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Thursday Aug 25, 2016 at 15:39 GMT

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

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Thursday Aug 25, 2016 at 18:34 GMT

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.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Thursday Aug 25, 2016 at 21:37 GMT

Have a look again guys, I've ripped out the spawn stuff into a separate function so the condition is much more readable. Also used fs.readJson which is used in Brackets to read package.json contents on multiple places (require does caching, we don't want that here).

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Tuesday Sep 20, 2016 at 21:27 GMT

Since 1.9 is too far away, can this be reviewed / merged for 1.8?

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Thursday Oct 13, 2016 at 04:02 GMT

sorry I missed this one... ready to re-review@ficristo

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Thursday Oct 13, 2016 at 19:14 GMT

@zaggino I downloaded your extensions brackets-coffeelint from github and then installed it using drag&drop. And worked great.
Then I modifyed the package.json of my local copy to add a dependency that doesn't exists.
I used again the drag&drop of the new zip and it failed to install, as expected.
What I feel a bit odd is that it removed the installation of my previous working copy. While I understand this behaviour when installing a new extension is a bit disappointing when you update an existing one.
Can we done something in this case? Sorry to realize this behaviour only now.

You should update npm-shrinkwrap.json.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Monday Oct 17, 2016 at 00:47 GMT

@ficristo updated shrinkwrap and fixed the issue you've mentioned, please re-review

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Monday Oct 17, 2016 at 17:52 GMT

Nice!
@petetnt, do you want to take another look?
@zaggino I'm not sure if it is necessary, but please open a new issue to update brackets-registry.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Monday Oct 17, 2016 at 22:49 GMT

@ficristo Open what issue? Update how? Only thing that pops to my mind would be some kind of UI message - this extension has npm dependencies? Or am I missing something, I'm not familiar with the registry code.

@core-ai-bot
Copy link
Member Author

Comment by madanbn
Tuesday Oct 18, 2016 at 05:15 GMT

moving it out to 1.9 release as we are closer to the 1.8 release time frame.

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Tuesday Oct 18, 2016 at 05:30 GMT

@zaggino actually I'm not particular familiar either, but in the registry there is the deps folder which contains a zip of the extensibily folder of brackets. I don't have any idea how it is generated but seems to me that it has to be keep in sync with brackets code.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Tuesday Oct 18, 2016 at 06:02 GMT

Right, had a look, I'll resolve this too.

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Monday Nov 21, 2016 at 11:33 GMT

Anything missing for this except brackets-registry PR ?

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Tuesday Nov 29, 2016 at 22:44 GMT

image

@core-ai-bot
Copy link
Member Author

Comment by swmitra
Wednesday Nov 30, 2016 at 01:54 GMT

@zaggino The last pic has forced me to look at this PR :smile.
I will test the changes and merge the PR today.

@core-ai-bot
Copy link
Member Author

Comment by MiguelCastillo
Wednesday Nov 30, 2016 at 04:04 GMT

this looks good to me.@swmitra - I wont have time to test this tonight... so I am really interested to hear how your testing goes. if for some reason you can't do it, let me know as well and I will find cycle for tomorrow.

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Wednesday Dec 14, 2016 at 19:04 GMT

Today I was thinking if this will work if the user is behind a proxy: do we need to pass it to npm script?

@core-ai-bot
Copy link
Member Author

Comment by zaggino
Wednesday Dec 14, 2016 at 20:51 GMT

I believe that it will pick up whatever is configured for npm on the machine. npm's config is stored in user's folder so if the domain is running under the same user, everything should be allright.

@core-ai-bot
Copy link
Member Author

Comment by ficristo
Friday Feb 10, 2017 at 10:40 GMT

is configured for npm on the machine

Don't we use the npm version embedded in this PR? If so don't we need to pass the proxy somehow to npm?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@core-ai-bot