elidoran / cosmos-browserify

Browserify npm modules for client side in Meteor packages
MIT License
78 stars 12 forks source link

Error when adding npm module from commit #13

Closed IonutLaceanu closed 9 years ago

IonutLaceanu commented 9 years ago

The meteorhacks:npm package has a nice feature where it allows to add a npm module by using a commit link. However, when I try to use that together with browserify, it doesn't work. The module gets downloaded and added to npm-container, but when trying to start the app, I get this error:

events.js:72
        throw er; // Unhandled 'error' event
              ^
Error: Cannot find module 'material-ui' from '/Users/ionutlaceanu/personal/music/packages/npm-container/.npm/package'
  at /Users/ionutlaceanu/.meteor/packages/cosmos_browserify/.0.4.0.12hoset++os+web.browser+web.cordova/plugin.CosmosBrowserify.os/npm/CosmosBrowserify/node_modules/browserify/node_modules/resolve/lib/async.js:46:17
  at process (/Users/ionutlaceanu/.meteor/packages/cosmos_browserify/.0.4.0.12hoset++os+web.browser+web.cordova/plugin.CosmosBrowserify.os/npm/CosmosBrowserify/node_modules/browserify/node_modules/resolve/lib/async.js:173:43)
  at ondir (/Users/ionutlaceanu/.meteor/packages/cosmos_browserify/.0.4.0.12hoset++os+web.browser+web.cordova/plugin.CosmosBrowserify.os/npm/CosmosBrowserify/node_modules/browserify/node_modules/resolve/lib/async.js:188:17)
  at load (/Users/ionutlaceanu/.meteor/packages/cosmos_browserify/.0.4.0.12hoset++os+web.browser+web.cordova/plugin.CosmosBrowserify.os/npm/CosmosBrowserify/node_modules/browserify/node_modules/resolve/lib/async.js:69:43)
  at onex (/Users/ionutlaceanu/.meteor/packages/cosmos_browserify/.0.4.0.12hoset++os+web.browser+web.cordova/plugin.CosmosBrowserify.os/npm/CosmosBrowserify/node_modules/browserify/node_modules/resolve/lib/async.js:92:31)
  at /Users/ionutlaceanu/.meteor/packages/cosmos_browserify/.0.4.0.12hoset++os+web.browser+web.cordova/plugin.CosmosBrowserify.os/npm/CosmosBrowserify/node_modules/browserify/node_modules/resolve/lib/async.js:22:47
  at Object.oncomplete (fs.js:108:15)

The packages.json file looks like this:

{
  "material-ui": "https://github.com/callemall/material-ui/archive/f78deffce5d3ffb830d85462103672c5efb4dc78.tar.gz",
  "react-tap-event-plugin": "0.1.7",
  "externalify": "0.1.0",
  "react-material-icons": "1.0.0"
}

The application.browserify.js:

injectTapEventPlugin = require("react-tap-event-plugin");
mui = require("material-ui");
FileDownload = require('react-material-icons/icons/file/file-download');

If I replace the link with "0.9.2" everything works as it should.

It would be really nice if it would work, because many of the react-related packages are under active development, and it comes handy to be able to use a package containing some bugfixes without waiting for the release.

Thanks, and keep up the good work!

elidoran commented 9 years ago

When I use the material-ui URL you gave the material-ui folder in node_modules doesn't have a lib folder in it. When I use 0.9.2 it does. The module's package.json file has:

It seems the URL you're using gives you the source code but it's not built into a distribution. When using 0.9.2 we get the published (built) version.

So, when browserify tries to use the material-ui module it reads the package.json file and then can't find the lib folder.

meteorhacks:npm is passing on the commit URL to Meteor's Npm.depend() function. It seems Meteor's NPM support would need to build the NPM module when downloading a source version instead of a published version.

IonutLaceanu commented 9 years ago

Ok, got it. Thanks!

mbrookes commented 9 years ago

@IonutLaceanu - did you open an issue for this with Meteor?

mbrookes commented 9 years ago

Hi @elidoran - I opened an issue for this over at the Meteor repo, and the guys there were very responsive, but suggested this might need to handled by a browserify transform. Any thoughts on whether that's possible? Thanks!

elidoran commented 9 years ago

@mbrookes I replied to the Meteor issue with my babble :) Basically, i think:

  1. it's possible to make a transform to do it and allow dev's to specify how to build a module
  2. it's better to support npm module work in Meteor's npm support for both client and server scope
  3. building npm modules is a varied enough activity it may be better to expect developers to build the modules manually for Meteor to use it

I'm open to discussion :)

mbrookes commented 9 years ago

Thanks for giving this your thought, and for the feedback on here and on the Meteor issue. I agree with you that if you have to go to the trouble of specifying how to build in the transform, you might as well just run the build manually.

Linking to a specific commit is probably not a common enough occurrence to justify trying to automate the build, and now that I understand what's going on, I'm happy to let this rest. I'd close the issue here if it was open to start with! :smile: I'll leave the Meteor one for now to see what the response is to your feedback. Thanks!