{"action":"created","issue":{"url":"https://api.github.com/repos/adobe/brackets/issues/12006","labels_url":"https://api.github.com/repos/adobe/brackets/issues/12006/labels{/name}","comments_url":"https://api.github.com/repos/adobe/brackets/issues/12006/comments","events_url":"https://api.github.com/repos/adobe/brackets/issues/12006/events","html_url":"https://github.com/adobe/brackets/issues/12006","id":122223707,"number":12006,"title":"Unifying third-party dependency management (through npm)","user":{"login":"petetnt","id":7641760,"avatar_url":"https://avatars.githubusercontent.com/u/7641760?v=3","gravatar_id":"","url":"https://api.github.com/users/petetnt","html_url":"https://github.com/petetnt","followers_url":"https://api.github.com/users/petetnt/followers","following_url":"https://api.github.com/users/petetnt/following{/other_user}","gists_url":"https://api.github.com/users/petetnt/gists{/gist_id}","starred_url":"https://api.github.com/users/petetnt/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/petetnt/subscriptions","organizations_url":"https://api.github.com/users/petetnt/orgs","repos_url":"https://api.github.com/users/petetnt/repos","events_url":"https://api.github.com/users/petetnt/events{/privacy}","received_events_url":"https://api.github.com/users/petetnt/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":13,"created_at":"2015-12-15T09:01:45Z","updated_at":"2015-12-19T17:27:57Z","closed_at":null,"body":"### The issue\r\n\r\nCurrently there are at least four different ways managing third-party dependencies in the Brackets core:\r\n\r\n1. Git `submodules`\r\n  * https://github.com/adobe/brackets/tree/master/src/thirdparty\r\n  * https://github.com/adobe/brackets/tree/master/src/extensions/default/JSLint/thirdparty\r\n2. `node_modules` committed into the folders themselves\r\n  * https://github.com/adobe/brackets/tree/master/src/extensibility/node/node_modules\r\n  * most recently this: https://github.com/adobe/brackets/commit/224633c3e5104880118c5f4cbe565bbd34965541\r\n3. Plain dumps of the files\r\n  *  https://github.com/adobe/brackets/tree/master/src/thirdparty \r\n  *  https://github.com/adobe/brackets/tree/master/src/extensions/default/InlineColorEditor/thirdparty\r\n4. `devDependencies` for Brackets itself:\r\n  * https://github.com/adobe/brackets/blob/master/package.json#L15\r\n\r\nand so on. Obviously every method has it's merits, for example in https://github.com/adobe/brackets/issues/11726#issuecomment-142957877 we had discussion on the merits of having submodules. \r\n\r\nBut let's take @busykai's comment on the merits of having submodules in that issue:\r\n\r\n> @zaggino, if you don't have a fork you never have a space to maneuver. If I remember correctly, we needed a modified version of CodeMirror on numerous occasions. jslint is another example. If you're using an npm version (i.e. a publicly distributed version) -- you're stuck. By stuck I mean if you fix it in the dependency upstream, you have to wait for 1) your contribution to be reviewed and accepted; 2) new package to be promoted and published. Logistically, it does not make any sense to wait for any of that. I don't know why would you assume it would never happen, it happened many times now.\r\n\r\nNowadays 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 in `package.json`. When the upstream fix is ready, you can just change the dependency back and run `npm 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 current `JSLint` fork under https://github.com/adobe/brackets/tree/master/src/extensions/default/JSLint/thirdparty\r\n\r\n> This branch is 1 commit ahead, 223 commits behind douglascrockford:master\r\n \r\nWhich 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... \r\n\r\n### Proposed solution\r\n\r\nPlain and simple: picking the de-facto way and running with it: managing all the core dependencies of Brackets through `npm`.\r\n\r\n### Potential problems / drawbacks of handling everything through `npm`\r\n\r\nHandling all the dependencies through `npm` does have some small drawbacks that I can think of: \r\n\r\n#### Increased maintenance cost\r\nThe *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.\r\n\r\nThere might be some potential conflicts if some of the extensions / core features use different versions of the same dependencies to boot.\r\n\r\n#### Increased amount of third party files\r\nThis 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.\r\n\r\n#### Potential problems with different `npm` versions\r\nSomething could be borked with `npm@2` that's fixed with `npm@3`\r\n\r\n\r\n### Summary of the benefits\r\n\r\nThen consider the benefits:\r\n- Having all the current dependencies in one place\r\n- Unifying the way of introducing new third party dependencies (and removing unneeded once)\r\n- A lot smaller initial size of the repo\r\n- Simplifying the hacking process to `git clone url/to/brackets && npm install`\r\n- Not having to deal with with submodules (I once again had issues with them with #11948 when the submodule paths had changed)\r\n\r\n`npm@3` has neat `deduping` capabilities and flat tree methology, which also removes many pains from Windows users (see: https://github.com/adobe/brackets/pull/11988#issuecomment-164590971 and https://github.com/adobe/brackets/pull/11988#issuecomment-164641483)\r\n\r\nObviously this would need some of initial work but in the end would be totally worth it. Opinions?\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/adobe/brackets/issues/comments/166005753","html_url":"https://github.com/adobe/brackets/issues/12006#issuecomment-166005753","issue_url":"https://api.github.com/repos/adobe/brackets/issues/12006","id":166005753,"user":{"login":"petetnt","id":7641760,"avatar_url":"https://avatars.githubusercontent.com/u/7641760?v=3","gravatar_id":"","url":"https://api.github.com/users/petetnt","html_url":"https://github.com/petetnt","followers_url":"https://api.github.com/users/petetnt/followers","following_url":"https://api.github.com/users/petetnt/following{/other_user}","gists_url":"https://api.github.com/users/petetnt/gists{/gist_id}","starred_url":"https://api.github.com/users/petetnt/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/petetnt/subscriptions","organizations_url":"https://api.github.com/users/petetnt/orgs","repos_url":"https://api.github.com/users/petetnt/repos","events_url":"https://api.github.com/users/petetnt/events{/privacy}","received_events_url":"https://api.github.com/users/petetnt/received_events","type":"User","site_admin":false},"created_at":"2015-12-19T17:27:57Z","updated_at":"2015-12-19T17:27:57Z","body":"I had some spare time today so I made the proof-of-concept, which is available here: \r\n  - [Branch](https://github.com/petetnt/brackets/tree/petetnt/poc-thirdparty-with-npm)\r\n  - Commit: https://github.com/petetnt/brackets/commit/3f1b6a0beadcdc87e5af50ec0aacd8c5c3c3a6c5\r\n\r\nEven 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 version of the current master branch with 99% of the third party dependencies handled with `npm`. The missing pieces are as follows:\r\n - `path-utils` still missing the modularization, see #11726. I'll do this tomorrow just in case because the fix is trivial (and useful later).\r\n - `thirdparty/globmatch.js` is a mystery: this most likely should have been a Brackets core feature that requires `minimatch` and `fnmatch` in the first place. The \"fix\" is once again trivial.\r\n - `thirdparty/murmurhash3_gc.js` has a similar problem to `path-utils`: alternative implementations exist and should be trivial to swap with.\r\n - Didn't have the time to swap test-only deps under `tests/thirdparty`\r\n  - :alarm_clock: Didn't run tests in general so there might be something hiding I don't know of yet\r\n - There are third party modules under `src/widgets` I only noticed now\r\n - `JavascriptCodeHints` runs `tern` code in a worker. This requires a similar hack that `extensions`  in general already use, which I didn't do yet. See more info about the hack below.\r\n\r\nThe 1% of the `brackets not working` is due me updating `React` to 0.14 which breaks [FileTreeView#L362](https://github.com/petetnt/brackets/blob/petetnt/poc-thirdparty-with-npm/src/project/FileTreeView.js#L362) (and maybe some other parts there) - fix should be trivial and I'll do a separate PR for that anyway soon-ish.\r\n\r\nOther notes:\r\n- I upgraded all the third party modules except `CodeMirror` to the latest versions: only `react` has breaking changes and `JSLint` needs some additional configuring.\r\n- The `requirejs config` can be further simplified a lot if needed.\r\n- Build process hasn't been touched / fixed / implemented so don't bother trying to build a release version from this yet :fire_engine: \r\n- `jQuery`, `Mustache` and `less` are added with `<script>-tags` in index.html - Here they have been moved to [`src/main#L112`](https://github.com/petetnt/brackets/blob/petetnt/poc-thirdparty-with-npm/src/main.js#L40) and then exposed to window.\r\n  - These cannot be removed from the window scope any time soon as they will pretty much break _all_ the extensions. :crying_cat_face: Maybe for `2.0.0`, heh.\r\n- Deps are now just required with `require(\"foo\")` or `getModule(\"foo\")` instead of `require(\"thirdparty/foo\");`\r\n    - Deprecation warning could be thrown if an extension requires `thirdparty/foo` - They are still mapped to `thirdparty/` with `requirejs` config to maintain backwards compatibility. \r\n    - If the removal of \"thirdparty/\"-notation is too drastic in the requires, we can also just name the keys as \"thirdparty/foo\" in [`src/main.js`](https://github.com/petetnt/brackets/blob/petetnt/poc-thirdparty-with-npm/src/main.js#L40)\r\n- `ExtensionUtils` has a long running problem with not using the global require context, see [ExtensionLoader#L164](https://github.com/adobe/brackets/blob/master/src/utils/ExtensionLoader.js#L164)\r\n  - I fixed this in [ExtensionLoader.js#L69](https://github.com/petetnt/brackets/blob/3f1b6a0beadcdc87e5af50ec0aacd8c5c3c3a6c5/src/utils/ExtensionLoader.js#L69) - It's kinda ugly so most likely it would be better off to do in `utils/global` and expose the info as `brackets.getGlobalRequireConfig()`. `utils/global` already uses the semi-private `requirejs.s.contexts.` there, so using it there is relatively safe (see James Burges comment [here](https://groups.google.com/forum/#!topic/requirejs/Hf-qNmM0ceI)). Most likely worth a PR on it's own\r\n  - This is the hack/workaround I mentioned above that `Workers` need too. Basically `importScripts(\"src/utils/global\")` but I didn't have the time to test it yet.\r\n- [Notes on CodeMirror](https://github.com/adobe/brackets/wiki/Notes-on-CodeMirror) in the wiki is outdated, I'll fix this tomorrow. "}}