Skip to content
This repository has been archived by the owner on Jun 15, 2021. It is now read-only.

installing dependencies separately fails in some cases #63

Closed
coolaj86 opened this issue Aug 21, 2011 · 30 comments
Closed

installing dependencies separately fails in some cases #63

coolaj86 opened this issue Aug 21, 2011 · 30 comments

Comments

@coolaj86
Copy link
Contributor

Works

ender build jQuery campusbooks

Fails

ender build jQuery ahr2 campusbooks

Welcome to ENDER - The no-library library
-----------------------------------------
installing packages: "ender-js jQuery campusbooks"...
this can take a minute...
successfully finished installing packages
assembling packages...
The package.json for campusbooks/ahr2 could not be found.
ender.js successfully built!
ender.min.js successfully built!

Additionally, this reports a successful build when it wasn't actually successful

@fat
Copy link
Contributor

fat commented Sep 18, 2011

lame -- i'm going to look into this today

@ded
Copy link
Member

ded commented Sep 18, 2011

possible related note. i've noticed recently (and not only me, but co-workers) that sometimes the ender-js client isn't first in the source... making things like ender being undefined when loaded in a browser. i thought that wasn't suppose to be async still?

@fat
Copy link
Contributor

fat commented Sep 18, 2011

yeah -- i'm looking at that now. Also, everything is still async.

@fat
Copy link
Contributor

fat commented Sep 18, 2011

this should work for you as of v0.6.6. Can you please test and reopen if you run into any issue.

(also, a side note -- i've taken the packageJSON.main default of index.json out for this release. The reason being is that npm wasn't forcing people to specify a module main, and the more i thought about it, the more i didn't like enforcing it either. This is particularly useful for people who are using package.json to define larger libraries (composed of smaller libs) which don't actually have any content of there own. for example ender's jeesh.

@fat fat closed this as completed Sep 18, 2011
@coolaj86
Copy link
Contributor Author

How about trying index.js, but gracefully failing.

if (!main) {
  try {
    module = require(path + '/index')
  } catch(e) {
    // this might be a meta package
  }
} else {
  module = require(path + '/' + main)
}

p.s. should just leave as index since there's also index.js, index.node, and index.coffee which are supported (obviously .node won't work in the browser, but .coffee could be if the right module is included).

@fat
Copy link
Contributor

fat commented Sep 18, 2011

Hm... is this a standard? Could you point to it somewhere?

The problem is, we alert the user if a defined main is missing and i think it would be a nontrivial change to add this.

@coolaj86
Copy link
Contributor Author

Is what a standard? using index rather than index.js? Or that index.js is the default?

When you require a package in node, it looks for modulename.js, modulename/index.node, modulename/index.js, and if you have the coffee compiler module loaded I believe it also looks for modulename/index.coffee - or at leaste I've noticed coffescript guys using index.coffee in their github repos.

Alerting that the problem is possibly a missing package.json.main is good, but since many users are using modules which they haven't written and not all authors repsond to pull requests quickly, I still think it would be best to try require('./index') for their convenience and give the warning if it isn't found.

@fat
Copy link
Contributor

fat commented Sep 18, 2011

alright -- i opened a new issue about it #80

@coolaj86
Copy link
Contributor Author

Did you push the changes to npm? I'm still having the same problem and I have run npm update -g ender

@fat
Copy link
Contributor

fat commented Sep 24, 2011

what version of ender are you running after update?

@coolaj86
Copy link
Contributor Author

npm info ender show 0.6.6

ender --version doesn't exist

@fat
Copy link
Contributor

fat commented Sep 24, 2011

that's true -- i should add --version ... for now i think npm ls -g | grep ender is your best bet.

Anyways, with 0.6.6 this ender build jQuery ahr2 campusbooks builds with no errors... are you getting errors in your output?

@coolaj86
Copy link
Contributor Author

Heres' my mkender.sh:

rm -rf node_modules
ender build \
  underscore \
  jQuery \
  edate \
  assert \
  amionline \
  futures \
  json-tables \
  ./ender-modules/app-local-storage-models \
  ./ender-modules/app-data-events \
  ./ender-modules/app-db \
  ./ender-modules/app-products-grid

The local modules have the correct dependencies in package.json as well.

It seems like the order in which the modules load is non-deterministic.

@coolaj86
Copy link
Contributor Author

I just tried this without the local modules and it actually wrote underscore in the file before it loaded ender-js such that provide is not defined. Then I ran it again and it worked the second time. Seems very non-deterministic.

rm -rf node_modules
ender build \
  underscore \
  jQuery \
  edate \
  assert \
  amionline \
  futures \
  json-tables

@fat fat reopened this Sep 25, 2011
@fat
Copy link
Contributor

fat commented Sep 25, 2011

The fact that underscore was before ender-js makes me think you are on ender 0.6.5 -- are you sure you're on 0.6.6?

@fat
Copy link
Contributor

fat commented Sep 25, 2011

i just published a new version of ender which has a version method.... can you update to ender 0.6.7 and run ender --version

@fat
Copy link
Contributor

fat commented Sep 25, 2011

ender builds packages asynchronously, which means if you do something like this:

$ ender build underscore jQuery edate assert amionline futures json-tables

You aren't guaranteed the order in which you specify packages.

The only guaranteed order is related to specific dependency maps. If any of those depend on order, that should be specified in the relevant package.json. does that make sense?

ender.js should always be first however, and i think i might have just uncovered a tiny bug with that.

@coolaj86
Copy link
Contributor Author

Hmm... either it's catching an error that exists in my code that was previously causing things to fail silently or it's more broken.

npm update -g ender
ender --version
Welcome to ENDER - The no-library library
-----------------------------------------
Active Version: v0.6.8

testing against ahr2

rm -rf node_modules
ender build ahr2
Welcome to ENDER - The no-library library
-----------------------------------------
installing packages: "ender-js ahr2"...
this can take a minute...
something went wrong install your packages!

Is there a way to output more information?

I've also tested other things, but they're still loading in the wrong order.

P.S. Both Jameson (beatgammit) and I would like to get more involved with Ender, but the coding styling is very foreign to us and hard to wrap our heads around.

@coolaj86
Copy link
Contributor Author

I don't know how you're calculating dependencies, but I think the best way to do it would be to store dependencies by depth

NOTE: This is working, tested code.

If the dependency tree looks something like this:

// simply have an empty object rather than -1, 0, etc
// This way `Object.keys(deps).forEach(fn)` will simply be a noop on `{}`,
// which is much more intuitive than `if {} else`ing on -1 and 0
// whether or not a module is installed can be handled elsewhere (as you will see later on)
var depTree = {
    "foo": {
        "bar": {}
    }
  , "baz": {
        "bar": {}
      , "qux": {}
      , "quux": {
            "bar": {}
          , "corge": {
                "bar": {}
            }
        }
    }
};

You could traverse it like this

function traverseAllDeps(depTree) {
  var depth = 0
    , allDeps = {}
    ;

  function traverseDeps(deps) {
    depth += 1;

    Object.keys(deps).forEach(function (depName) {
      var childDeps = deps[depName]
        ;

      allDeps[depth] = allDeps[depth] || [];
      allDeps[depth].push(depName);

      if (childDeps) {
        traverseDeps(childDeps);
      }
    });

    depth -= 1;
  }

  traverseDeps(depTree);
  return allDeps;
}

var depthTree = traverseAllDeps(depTree)

The result of the traversal will be this:

JSON.stringify(depthTree, null, '  ');
{
    "1": [ "foo", "baz" ]
  , "2": [ "bar", "bar", "qux", "quux" ]
  , "3": [ "bar", "corge" ]
  , "4": [ "bar" ]
}

And then you can reduce that result into a simple list like so:

function decideWhichToInstall(depthTree) {
  var order = 0
    , handled = {}
    , useInOrder = []
    ;

  // it's important to sort and reverse the key listing so that 
  // the most deeply depended on modules are always installed first and listed first in ender.js
  Object.keys(depthTree).sort().reverse().forEach(function (depth) {
    var depNames = depthTree[depth]
      ;

    depNames.forEach(function (depName) {
      if (!handled[depName]) {
        handled[depName] = true;
        useInOrder.push(depName);
      }
    });

  });

  return useInOrder;
}

var useInOrder = decideWhichToInstall(depthTree);

As you can see, this produces a very clean array of which order the modules must be used and installed in:

JSON.stringify(useInOrder);
["bar", "corge", "qux", "quux", "foo", "baz"]

Then you can do the module installs and write out to ender.js:

function doInstall(next, depName) {
  // whatever needs to happen for the install to complete goes here
  // maybe don't reinstall if you don't need to 
  // (but it's probably best to always reinstall anyway)
  cb();
}

// require('Array.prototype.forEachAsync')
useInOrder.forEachAsync(doInstall).then(function () {
  useInOrder.forEach(function (dep) {
    // replace this log with the function that writes the module out to ender.js
    console.log(dep);
  });
});    

Some may argue that you could do this in fewer lines of code or more efficiently with fewer loops, etc.
However, it works in a consistent fashion and I believe it's easy to understand (aside from the fact that I didn't
choose the best names for all of the variables).

What do you think about this approach?

If you think it's good and you give me some guidance then I'll dig around and try to implement it.

@fat
Copy link
Contributor

fat commented Sep 25, 2011

I just pushed some code which fixed the regression from earlier -- i had set the version on the process object like a n00b which apparently caused your module to fail internally inside npm. Weird it didn't effect other modules o_O

Anyhow, i added a --debug flag just now and started adding some checks where if it's present it will throw errors when encountered instead of just failing silently. There's still a lot more to be added to the debug, but it's better than nothing.

As far as the dependencies go, i believe our current implementation is actually working and not too different from what you've done here. The -1 is very valuable for accounting for paths with missing dependencies. Npm is very smart about installing modules but also fairly closed off with what it exposes to you with regard to what it actually installs. Because of this, the -1 gives you a clue as to if a package exists at a particular location or not. I chose a 0 rather than an empty object to signify a package with no dependencies because it's an easier conditional check. Otherwise the tree is very simliar to what you proposed.

That said, if you feel like going down that road I'd be happy to help you along/take any advice. All that code lives in Ender.file.js.

Before you do that though, i would give your install one more shot, because it throws no errors for me during the build process and everything seems good.

@fat
Copy link
Contributor

fat commented Sep 25, 2011

Also, reading back over that

the coding styling is very foreign to us and hard to wrap our heads around

sorry to hear :(

What sort of things are most unusual? I think this particular code/logic is probably the most challenging in the cli. I'd be happy to work through a better/simpler solution though. If you start with the assemble method you let me know how far you get and if you have questions.

@coolaj86
Copy link
Contributor Author

I'm not sure what I'm doing wrong, if anything, but I just can't get this to build correctly.

Can I give you a tarball of the local modules and my mkender.sh to see if you can or can't reproduce the issue on your machine?

@coolaj86
Copy link
Contributor Author

As for the coding style:

It seems that most node.js modules have a pattern similar to this:

(function () {
  "use strict";

  var MyModule = module.exports; // or {};

  function somePrivateFunction () {
  }

  MyModule.someMember = function () {};
  MyModule.someOtherMember = function () {};

  module.exports = MyModule; // or this is already taken care of at the top
}());

actually, far too few people are using "use strict"; despite how many errors it can catch and the supposed speed increase, but I always do that.

You guys seem to follow the jQuery style, which I haven't spend a lot of time with. I don't understand the background and purpose of the object-wrap convention. If I did it probably would seem as natural to me as the above, but something about it is just harder to follow. I can't quite put my finger on it.

var MyModule = {
  someFunc: function () {},
  someOtherFunc: function () {
    this.someFunc();
  }
};

and also I've seen this type of thing in ender where if, while, and other blocks in one line;

if (true == 'true') someObj && someObj.someFunc(someVar) || someFunc(null, true)

which is a bit more difficult to understand than

if (true == 'true') {
  if (someObj) {
    someObj.someFunc(someVar);
  } else {
    someFunc(null, true);
  }
}

and I understand that some developers condensing their code to save bytes, but I believe the newer compilers such as uglify-js do that sort of thing for you when it's compiled.

Also, I very rarely succumb to javascript-foo, so I'm not as familiar with the things that happen to work (but don't make sense unless you've spent a lot of time on wtfjs) -- like using bitwise operators changes strings and objects to numbers, etc.

Those are the main things that confuse me about the style.

@coolaj86
Copy link
Contributor Author

Oh, I haven't poked around with assemble yet, but I'll take a peek tonight.

@coolaj86
Copy link
Contributor Author

I've been working on a rewrite of the build process that solves the sometimes out-of-order issue, the multiple file per package issue, the package.ender.dependencies issue, and some other things.

It's basically an API to get the dependency, file, and order-of-install info that would work great as a submodule for any packaging system.

I'll post back when it's fully functional.

@fat
Copy link
Contributor

fat commented Oct 13, 2011

awesome, i'm looking forward to it! :)

@coolaj86
Copy link
Contributor Author

Okay, here's the first part: https://github.com/coolaj86/node-pakman.

This packages a single module, including any subrequires, but not including npm deps.

I'm finishing up with the code to recursively include npm deps.

@coolaj86
Copy link
Contributor Author

I'm so close to having a fully working example of pakman.

Last night as I was going to bed I found that I wasn't properly handling package.json.main for some packages. I think I'll have the fix within an hour or two.

@coolaj86
Copy link
Contributor Author

@fat,

So I've got http://github.com/coolaj86/node-pakman as an API and http://github.com/coolaj86/node-pakmanager as an example on top of it.

Tell me what you think.

I'd like to get this merged into ender - perhaps an ender 2.0.

@rvagg
Copy link
Member

rvagg commented May 9, 2012

a lot of these dependency issues seem to be sorted out in the 1.0-wip branch now.

@rvagg rvagg closed this as completed May 9, 2012
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants