RaveJS / rave

Zero-configuration application bootstrap and development
278 stars 8 forks source link

Location and main of npm packages #24

Closed KidkArolis closed 10 years ago

KidkArolis commented 10 years ago

I've noticed that when configuring the location and main of npm packages, location is set to the dir where the main file is located instead of setting it to the root of the package (which is how I think it works in node).

mainPath = path.splitDirAndFile(descr.main);
if (mainPath[0]) {
  descr.location = path.joinPaths(descr.location, mainPath[0]);
  descr.main = mainPath[1];
}

https://github.com/RaveJS/rave/blob/master/lib/metadata/npm.js#L28-L32

I was wondering if that means that no file within the descr.location will be able to require files from higher up in the package, e.g. if main is lib/index.js in node it can still do require("../foo/another-file.js"). What will happen in rave?

I could investigate further and submit a fix if you'd like.

unscriptable commented 10 years ago

iirc, this code was copy-pasted from the bower metadata transform to deal with bower packages that don't place modules at the top level. Perhaps this is much rarer in the npm world and we should remove this logic? Thoughts anyone? cc @briancavalier @scothis @phated

unscriptable commented 10 years ago

I was wondering if that means that no file within the descr.location will be able to require files from higher up in the package, e.g. if main is lib/index.js in node it can still do require("../foo/another-file.js"). What will happen in rave?

This should still work.

KidkArolis commented 10 years ago

I think it doesn't. Just tried require("react-tools"). One of it's dependencies, jstransform, has "main": "src/jstransform". But react-tools also calls require('jstransform/visitors/es6-arrow-function-visitors') which fails to resolve since the location is set to jstransform/src. Removing the 4 lines in the above snippet fixes the issue.

(of course then I ran into a different issue of native node modules like require("path"), but that's a different topic)

phated commented 10 years ago

I think the logic stands for bower but should be removed for node. Node has very rigid package loading rules, but bower less so. My suggestion for the bower default location change is due to that laxness.