Yep, I fully support this refactoring. Just wanted to encourage that you try the publish & install flow earlier rather than later in case the instanceof thing does work.
On Fri, Jun 6, 2014 at 3:42 PM, Michal Mocny <[email protected]> wrote: > Anis: sounds good. Thats was the original intent, and cordova-lib repo was > the first step. Also agree that once we have a package that really is well > decoupled from the rest, we can consider moving to independent repos, > especially if it has some utility on its own. > > (The CordovaError point was just one issue we faced when we last > contemplated how to do this, but we are less experienced here.) > > -Michal > > > On Fri, Jun 6, 2014 at 3:25 PM, Lorin Beer <[email protected]> wrote: > > > Here's a wip for the breakout of the config parser module > > > > https://github.com/lorinbeer/cordova-lib/tree/configparser_module > > > > still need to tidy up the referencing but feedback welcome > > > > On Fri, Jun 6, 2014 at 11:03 AM, Anis KADRI <[email protected]> > wrote: > > > The goal is to have independent modules that can be consumed. > > > > > > Andrew, your second example reflects what we want to achieve. > Obviously, > > > there will be more than CordovaError. Brian and I think that we can > keep > > > the current cordova-lib repository and create folders for each module > > (each > > > with its own dependencies and package.json). Eventually, there could be > > > more repositories if we chose to have them. > > > > > > Right now if you just want one piece of functionality from either > plugman > > > or cli you need the whole cordova-lib module which is extremely > > unfortunate > > > and pretty silly. > > > > > > > > > On Fri, Jun 6, 2014 at 10:55 AM, Michal Mocny <[email protected]> > > wrote: > > > > > >> Additionally, I think it will only be a problem when each of the > modules > > >> call into one another. I.e. when cordova-cli calls into > cordova-plugman > > >> and that throws a CordovaError which is caught by cordova-cli and then > > >> compared with instanceof to its own version.. Its not a problem in > > >> isolation. > > >> > > >> This problem usually comes up with singletons / internal caching, such > > as > > >> reading config files from disk once and then modifying in-memory, but > > I'm > > >> not sure if we actyally have that problem. > > >> > > >> -Michal > > >> > > >> > > >> On Fri, Jun 6, 2014 at 1:12 PM, Andrew Grieve <[email protected]> > > >> wrote: > > >> > > >> > On Fri, Jun 6, 2014 at 12:37 PM, Brian LeRoux <[email protected]> wrote: > > >> > > > >> > > Ya I spoke to Issac a bit about this last week and he wasn't being > > >> > entirely > > >> > > facetious. One path that apparently works well is to treat root > > >> > > `./node_modules` as something npm manages and then nested > > node_modules > > >> > for > > >> > > userspace stuff. > > >> > > > > >> > > eg. `./src/node_modules` and then `var foo = require('src/foo')` > > >> > > > > >> > > Not a huge win over just using relative paths so meh. > > >> > > > > >> > > I don't understand why instanceof wouldn't work? > > >> > > > > >> > > > >> > You probably have a better understanding of this than I do... But: > > >> > - There's no problem when running locally, since we've defined the > > >> > structure of node_modules to work, > > >> > > > >> > But when npm install is run on an end-users machine, if it ends up > > like: > > >> > > > >> > cordova-lib/node_modules/cordova-plugman/node_modules/CordovaError > > >> > cordova-lib/node_modules/cordova-cli/node_modules/CordovaError > > >> > cordova-lib/node_modules/CordovaError > > >> > > > >> > then there are now 3 CordovaError constructors, so instanceof will > not > > >> work > > >> > between them. > > >> > > > >> > Now, if npm sees that all packages call for the same version of > > >> > CordovaError, and install them like: > > >> > > > >> > cordova-lib/node_modules/cordova-plugman > > >> > cordova-lib/node_modules/cordova-cli > > >> > cordova-lib/node_modules/CordovaError > > >> > > > >> > then all is well. > > >> > > > >> > Easy to answer with a quick test :) > > >> > > > >> > > > >> > > > >> > > > > >> > > > > >> > > On Fri, Jun 6, 2014 at 9:14 AM, Andrew Grieve < > [email protected] > > > > > >> > > wrote: > > >> > > > > >> > > > Exciting! I tried the "check in node_modules" approach in > > >> > > > cordova-app-harness and it works really well there: > > >> > > > > > >> > https://github.com/apache/cordova-app-harness/tree/master/harness-push > > >> > > > It's a simpler case since there's a strict parent/child > > relationship. > > >> > > > > > >> > > > The main unknown for me is how to make CordovaError work as its > > own > > >> > > module > > >> > > > since we'll require that there is never multiple copies of the > > module > > >> > > when > > >> > > > the end-user types "npm install" (or else the instanceof check > > won't > > >> > > work). > > >> > > > I *think* that npm does the right thing so long as all of the > > >> packages > > >> > > use > > >> > > > the same version constraints for it, but I haven't tested it. > > >> > > > > > >> > > > > > >> > > > On Fri, Jun 6, 2014 at 11:37 AM, Michal Mocny < > > [email protected]> > > >> > > wrote: > > >> > > > > > >> > > > > Brian: sounds good. Looking forward to see where we get. > > >> > > > > > > >> > > > > -Michal > > >> > > > > > > >> > > > > > > >> > > > > On Fri, Jun 6, 2014 at 11:36 AM, Brian LeRoux <[email protected]> > > wrote: > > >> > > > > > > >> > > > > > @Michal: we're going to start extracting things to their own > > >> > modules. > > >> > > > (As > > >> > > > > > discussed.) We're going to start small and simple: > > CordovaError, > > >> > > > > > SuperSpawn, etc. But before we do that we'll try to merge up > > as > > >> > many > > >> > > > PRs > > >> > > > > as > > >> > > > > > possible. Ideally there are none when we get rolling. (Next > > >> week.) > > >> > > > > > > > >> > > > > > Many repos (or just small modules) would isolate the changes > > >> making > > >> > > it > > >> > > > > > easier to refactor. Anyhow: I'm tired of advocating that > > design > > >> > > pattern > > >> > > > > > choice and I'm sure you're tired of hearing about it! > > >> > > > > > > > >> > > > > > > > >> > > > > > On Thu, Jun 5, 2014 at 6:04 PM, Michal Mocny < > > >> [email protected]> > > >> > > > > wrote: > > >> > > > > > > > >> > > > > > > On Thu, Jun 5, 2014 at 6:47 PM, Brian LeRoux <[email protected]> > > >> wrote: > > >> > > > > > > > > >> > > > > > > > we're about to do some heavy refactoring on > cordova-lib…it > > >> > would > > >> > > > > appear > > >> > > > > > > > many ppl are working on it atm: > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > Who/how/what/why? > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > > >> > > > > > > > https://github.com/apache/cordova-lib/pulls > > >> > > > > > > > > > >> > > > > > > > I'm afraid to find out how many *aren't* open PR's …but > > this > > >> > > > > certainly > > >> > > > > > > > illustrates the problem with one giant repo with all our > > code > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > If you are refactoring the code in a way that breaks PR > from > > >> > > > applying, > > >> > > > > I > > >> > > > > > > don't think multiple repos would make a difference. > > Probably > > >> > this > > >> > > > way > > >> > > > > is > > >> > > > > > > easier if you were making changes that needed to touch > > multiple > > >> > > > repos. > > >> > > > > > But > > >> > > > > > > this conversation feels sorta Deja Vu. > > >> > > > > > > > > >> > > > > > > -Michal > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > >> > > >
