search

rxaviers / cldr-data-npm

Npm module for Unicode CLDR JSON data
MIT License
42 stars 28 forks source link

cldr-data install.js is broken wrt. peer dependencies #29

Closed sam-github closed 8 years ago

sam-github commented 8 years ago

It has three problems:

  1. it fails, but then exits with status 0: https://github.com/rxaviers/cldr-data-npm/blob/master/install.js#L52, this is incorrect. Failing scripts must exit with a non-0 status to indicate failure.
  2. its determination of whether it is installing because of a peer dependency appears to be a protection to be future proof with npm3... but actually, it fails with npm3
  3. the message is not grammatically correct English, but that's pretty minor

Our scenario is this:

This is all correct. cldr-data is installed via direct dependency, NOT via the globalize peerDependency, but cldr-data doesn't download, and after printing a message, reports success via its exit status.

cldr-data's gets installed by npm3 at the top-level, using the flattening, and this appears to confuse the script.

sam-github commented 8 years ago

This is the root of the bug:

https://github.com/rxaviers/cldr-data-npm/blob/master/install.js#L20 is assuming a very specific npm package layout, a layout that isn't guaranteed by npm@2 under all conditions, and is not how npm@3 installs packages, it flattens them.

Personally, I don't think the problems of this check out-weigh any benefits. I'd fix it, but I'm not actually sure how that would be done. Its not clear that npm allows a package to know "why" it was installed, and I believe all attempts to determine this will run afoul of npm's install/package layout algorithm in at least some corner cases.

marcelklehr commented 8 years ago

I just hit this as well. Very annoying. Solution would be to just throw out the check for peer dependency.