search

spine / hem

Bundler for Node/CommonJS/Web Apps
MIT License
281 stars 71 forks source link

Potential fix against #19 #69

Closed mjtorn closed 11 years ago

mjtorn commented 11 years ago

This seems to work for us against issue #19 - where commonjs names are resolved so they don't work in other code.

Is this enough of a fix or is something else required?

cengebretson commented 11 years ago

Thanks for the pull request. I probably will be working on hem this week, I will try to set aside some time for testing this pull request and integrating it in.

mjtorn commented 11 years ago

Sure. We would appreciate not having our own fork in use any more than we have to. If this PR bounces, we'll just do wrappers.

The philosophy is whether or not un-magicking names like this is wanted in hem or not. IMO this is a worthwhile patch but I may not be deep enough in the scene to know everything :)

cengebretson commented 11 years ago

Doesn't look like I am can automatically merge the changes in, but they look pretty simple so I can take care of it. I'm actually working a hem a bit this week so I'll try to get that in and test it, hopefully get a minor hem update out next week.

I think this fix is good for now. I'm trying to refactor a few things in hem and one of the things I want to do is improve the node_module import process, in that it will respect what is set in package.json instead of just assuming index.js. I prefer that approach vs trying to fork the other projects and making wrappers around them so they work with hem.

thanks!

cengebretson commented 11 years ago

I brought this into the master branch for now, will try to get a new minor hem release out soon. I think for the long term I am going to look at something that can smartly handle what the module's package json declares as main. Thanks for the pull request!