where is that offending bit…and what is the intent behind it. outside of pretty stack traces I'm still not convinced of the utility of building out own own exceptions
On Sat, Jun 7, 2014 at 6:19 AM, Andrew Grieve <[email protected]> wrote: > 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 > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > >> > > > > > >
