-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Unifying third-party dependency management (through npm) #12006
Comments
As stated a few times before, I'm all in for using |
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) |
The actual changes needed would be:
|
@petetnt Thank you for the long explanation. |
@ficristo Default extensions would/should use the 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. |
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
@petetnt, I would revisit the problem you're trying to solve. If the problem is Brackets uses multiple ways to include 3rd party dependencies, then it's not a problem. It's actually OK. Each way of including a dependency has its reason. If the problem is you not liking submodules, then it's not objective. To start off, we need a good and objective problem statement. P.S. If you like to, you could try moving |
@busykai , thanks for your reply.
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 |
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. |
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 #11988 (comment) - why do we want to commit 10MB of |
@busykai I understand where you are coming from, but I don't agree with things like difficulty of |
@zaggino, my point. I'm not against npm (or bower or whatever other package manager). I'm against unification (simply because it's impossible). So I do agree with you.
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 @petetnt argues. Just take a look at the title of this issue.
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, @petetnt , the only thing you need to do is to try and materialize your ideas. If you're running in |
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). |
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:
|
Lately I'm interested in updating the dependencies under test/. |
Well - I didn't know I was walking into a house in flames! @busykai - friend, let your guard down a bit. Your comments almost read as attacking people who are spending their free time and effort to legitimately try to help us get to a better place! We are all playing for the same team :) Your points are spot on. It is great to bring them forward so that we are reminded of why certain things are done the way they are in Brackets. But two questions are. 1) Can we do better? I know the answer is yes. 2) Is having consistency a good thing? (Call it unification if you want) If yes, then the follow up question is how can we get there? @petetnt is giving us a damn good option that I don't see anyone else take the time to do. Let's figure out how we can help him make this happen. @ ya'll - If there are concerns or issues, then let's help get answers to them. Not use them against anyone to inhibit progress. 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 @petetnt I think that we can divide an conquer, instead of wholesale this because it will get unwieldy. What about migrating tern and codemirror, which we need right away in order to move forward with a few other PRs. To me these two are higher priority on the list. Let's work with @marcelgerber since he's leading the process of updating those two; @busykai and I am helping where we can. I would suggest breaking down this process even further. Perhaps we can add codemirror and tern to our package.json, and use gruntfile.js to copy the necessary files to proper place in thirdparty or whatever default extension; we already do that for codemirror anyways. This will give us npm for two very important modules with minimal code changes. Once we merge tern and codemirror, we can tackle more dependencies and perhaps reading directly out of 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. |
@MiguelCastillo, it was not my intention to attack anyone (I don't believe I did). I am sorry the discussion could be read this way. I do question the motivation for the proposed change and lack of a problem statement. I still do not understand 1) what's the problem being solved by this; 2) how resulting end state is any better than what we have right now. I also encouraged @petetnt to try doing actual change instead of arguing "in the air" (which he did, which is awesome). 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). |
Thanks for your comments @busykai and @MiguelCastillo! I agree with the divide and conquer approach @MiguelCastillo suggested too. Part of the work done in this proof-of-concept have been already merged, such as Updating React and Immutable (#12035) while others are still pending (such as update Mustache #12034, moving PathUtils out of global scope #12203 and using the global config for ExtensionLoader #12041). Some are yet to be separated, such moving |
I wonder if we could treat the PS: There is a PR to remove Mustache from the global scope. |
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. |
FWIW I'd love to get rid of |
@petetnt same with me. I think it could be done in stages, too, since webpack is fine with AMD modules, and you could migrate to a CommonJS pattern down the road. 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. |
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. |
Webpack 2 supports dynamic imports (import()) so it should be very much possible. |
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 #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: #11988 (comment) and #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: