-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
Unifying third-party dependency management (through npm) #13470
Comments
Comment by zaggino As stated a few times before, I'm all in for using |
Comment by ficristo While I'm totally in favor for this to happen I'm trying to think some drawbacks. AFAIK Brackets architecture is divided in two: a part that resembles the browser and the part using node. For the pros I will add that if we want to support node 4 having the dependencies up-to-date should somewhat a requirement. (Maybe they will work as is but in recent versions the support should be more guaranteed) |
Comment by petetnt
The actual changes needed would be:
|
Comment by ficristo
|
Comment by petetnt
This change wouldn't affect the Node-features of Brackets itself, it would just unify the way the third party dependencies are handled inside the core. |
Comment by busykai A git submodule is a fully-functional repository you can work in, change branches and commit to, it's not just a dependency. It is many time (not always though) the case that you need to do development in parallel (take We can manage
I know that. It creates a problem rather than resolves one. Little bit of IMHO: in my experience, Also, And finally, this piece runs in Brackets'
this piece is used for CI in a headless Linux environment and ran in
P.S. If you like to, you could try moving |
Comment by petetnt
I'd like to emphasize that I am not championing using
CodeMirror is on All of them are also hosted online on a git based platform (in these instances, GitHub), which means we can also install them directly from the repos themselves: from any version, from any commit. If there's need for cherry picking, then we would host them on it's own repository, just like 'jslint': 'peterflynn/jslint#5a09b35' to install that exact version. Only 3rd party dep that I can find that isn't hosted directly on
Yes, shrinkwrapping is an option (which is relatively easy to do) and yes, super relaxed module versions can create issues. Obviously caution must be used when specifying (and updating!) the versions. That said, there is very little overlap in the sub-dependencies in Brackets (at least right now). Like I said previously, one of my motivations here is to prevent code rotting by actually encouraging updating the said modules.
Actually, let's look at the RequireJS docs on loading
By default Furthermore, there needs to be an certain distinction made here about
This might be an issue though, at which point I guess dumping the
I'd love to try it, but as my time has been quite tight as of late, I would like more comments from the owners, collaborators (like you) and community members, if such thing would be desirable before diving in because comments like yours are extremely valuable (even if you don't agree with me... yet 😸). Obviously I think it would be worth it and I am championing |
Comment by busykai I know
The doc you're referring to is about loading node modules using
You're missing my point. I know they are on
So is mine. Hence until someone actually creates a prototype which demonstrates how it's better, I'll refrain from the further discussion. Let me summarize what i think before I go: to me, the issue does not have a sound problem statement. You are trying to address a development-time configuration of Brackets repository which so far did not cause any inconvenience. IMHO, it's just not worth typing so much characters as we already did. I think it could be improved, I just don't see how it can be unified for all types of dependencies and execution contexts that Brackets has. I do not understand why it has to be unified in the first place. |
Comment by zaggino Guys, I'll be short, but this discussion shouldn't be about having only npm dependencies or having only git submodule dependencies. I believe we should have both, deciding on each separate case, what's more appropriate. Why are we resistant against npm dependencies? Please check out adobe/brackets#11988 (comment) - why do we want to commit 10MB of |
Comment by petetnt
|
Comment by busykai
FWIW, ESLint extension should be pre-installed, not embedded. So should be any other linter. I agree, we don't need all the node_modules garbage in git in your ESLint example. But let's assume assume it's in git, for the sake of argument... These are truly node dependencies. They are loaded in A note of caution: where use pretty old version of node and npm (did you use these to fetch the dependencies, by the way?). We'll have to upgrade them (and all the dependencies) and keep upgrading periodically for a sustainable npm-based solution. Now that all that io.js vs node.js storm is over, I think we could relatively safely switch to some 4.x.x node version. It's a lot of work though. Note, however, this is not what
I repeat this again. Because requirejs in browser context will not load from node_modules. Nor the modules will lookup and look up their dependencies. Again, |
Comment by zaggino Already made and attempt and will try to work on node 4.x.x -> adobe/brackets-shell#543 but ESLint thingy should be safe. You're right that I've downloaded dependencies with node4 but my brackets use 0.10 (as everyones) and everything works (there are no native dependencies in the tree). |
Comment by petetnt I had some spare time today so I made the proof-of-concept, which is available here: Information about the branch itselfEven if this will never materialize, I am rather happy with the PoC: I learned tons more of Brackets internals which I didn't know previously. This proof-of-concept is a 99% working, 100% backwards compatible and extension friendly version of the current master branch with 99% of the third party dependencies handled with
Missing piecesThe missing pieces are as follows:
What is broken
Other notes:
|
Comment by ficristo Lately I'm interested in updating the dependencies under test/. |
Comment by MiguelCastillo Well - I didn't know I was walking into a house in flames!
My perspective, with my limited knowledge of module and dependency management is that submodules are not a dependency/versioning management tool. And to me that's a huge part of the problem we are trying to solve. Making the workflow for updating dependencies smoother, and npm is arguably the best option for our ecosystem at the moment. As far as I am concerned, pointing dependencies to particular git SHAs (via submodules or npm dependencies) is asking for trouble. With currently available toolset, this should be the corner case and not our general workflow. For sure we can say that "lose" dependency versioning in npm is bad; because it generally is. But that's a moot point because that's an issue the community has already solved for a long time ago; this not a reason to not move to npm to manage dependencies. Ya'll have given a couple of good answers to this problem already :D
I do realize some things won't be easily moved over to npm. We can work through each individually and determine how and if we should move them over. Ultimately, it would be awesome to have a single workflow for managing dependencies. Let's shoot for that and we will make the rest exceptions to the rule if we must. |
Comment by busykai
I do not think, by the way, that "consistency is good" as a general universal rule. Anyways, I have voiced all I had to say in my comments above. Question of perceptions and taste are usually resolved by majority. To me, hearing more voices in favor than in opposition commands to just accept the reality (and all that comes with it). |
Comment by petetnt Thanks for your comments I agree with the divide and conquer approach |
Comment by ficristo I wonder if we could treat the PS: There is a PR to remove Mustache from the global scope. |
Comment by humphd I don't want to hijack this bug, but it seems like the best place to ask this. In our Mozilla fork, I'm considering moving to webpack vs. requirejs for doing our build. I see that you have recently made some moves in this direction, and I wonder if it's something you're considering doing en masse? I realize it might not make sense for you, especially with how extension loading is done currently, and how tightly it is bound to requirejs. For us, working in the browser, and with a limited set of pre-known extensions, size is really important. |
Comment by petetnt FWIW I'd love to get rid of |
Comment by humphd
As I say, I didn't mean to derail this bug, and just wanted to see what people were thinking. If you think this is something worth doing in general, maybe we could collaborate and do it in this repo vs. me doing it in our fork. I'd be willing to help with that work. |
Comment by zaggino Extensions are currently loaded dynamically after the after the Brackets start-up. Is this possible with commonjs-webpack? Or would it require us to pack the extensions before loading them? If we could get a proof of concept how to do this, I'd certainly vote for. |
Comment by petetnt Webpack 2 supports dynamic imports (import()) so it should be very much possible. |
Issue by petetnt
Tuesday Dec 15, 2015 at 09:01 GMT
Originally opened as adobe/brackets#12006
The issue
Currently there are at least four different ways managing third-party dependencies in the Brackets core:
submodules
node_modules
committed into the folders themselvesdevDependencies
for Brackets itself:and so on. Obviously every method has it's merits, for example in adobe/brackets#11726 (comment) we had discussion on the merits of having submodules.
But let's take
@
busykai's comment on the merits of having submodules in that issue:Nowadays you can install
npm install
from any repo, from any branch, from any commit, from a tarball, pretty much from everywhere. You don't have to wait for an upstream fix if you don't want to, just push your fork to your own repo and change the dependency inpackage.json
. When the upstream fix is ready, you can just change the dependency back and runnpm install
again. Sometimes you have to cherry pick commits or parts of them, and that's fine too. But hiding the third party dependencies to subfolders under subfolders easily lead (in my opinion) to heavily outdated dependencies and code rot. Just look at the currentJSLint
fork under https://github.com/adobe/brackets/tree/master/src/extensions/default/JSLint/thirdpartyWhich then leads into issues such as #12000. Who knows at what versions are the other dependencies at? At least I don't know, I assume fairly recent but I cannot be sure: hell, I cannot be sure what Brackets depends on without going through all the folders searching for
node_modules
,thirdparty
folders, ´thirdparty-foo.js` files...Proposed solution
Plain and simple: picking the de-facto way and running with it: managing all the core dependencies of Brackets through
npm
.Potential problems / drawbacks of handling everything through
npm
Handling all the dependencies through
npm
does have some small drawbacks that I can think of:Increased maintenance cost
The maintenance cost of Brackets core can potentially increase if the dependencies are constantly kept up-to-date: this of course applies to other ways of maintaining the thirdparty deps too, but the simplicity of actually maintaining the dependencies might actually encourage people keep them up-to-date, which could increase the said cost.
There might be some potential conflicts if some of the extensions / core features use different versions of the same dependencies to boot.
Increased amount of third party files
This mostly applies to the few examples where the actual third party files are just dumped into the folders, but this is mostly a non-issue, as most of the files are already submodules or fully committed
node_modules
anyway.Potential problems with different
npm
versionsSomething could be borked with
npm@2
that's fixed withnpm@3
Summary of the benefits
Then consider the benefits:
git clone url/to/brackets && npm install
npm@3
has neatdeduping
capabilities and flat tree methology, which also removes many pains from Windows users (see: adobe/brackets#11988 (comment) and adobe/brackets#11988 (comment))Obviously this would need some of initial work but in the end would be totally worth it. Opinions?
The text was updated successfully, but these errors were encountered: