openexchangerates / accounting.js

A lightweight JavaScript library for number, money and currency formatting - fully localisable, zero dependencies.
http://openexchangerates.github.io/accounting.js
MIT License
4.96k stars 530 forks source link

Patch to Added support for Titanium Appcelerator #32

Closed euforic closed 12 years ago

euforic commented 12 years ago

Simple addition for exporting as Titanium Appcelerator Module.

wjcrowcroft commented 12 years ago

With this patch, running in nodeJS, as far as I can see the if (typeof module !== 'undefined' && module.exports) { line will never be reached (exports is undefined)

Other than that, does this patch have any other applications than for Titanium Appcelerator? I'm very wary of adding framework-specific code in here (the CJS and AMD exporting are for multiple frameworks that follow a specification, as opposed to any single framework like RequireJS)

euforic commented 12 years ago

The patch is directly copied from a the patched added to underscore for the same reason https://github.com/documentcloud/underscore/blob/master/underscore.js#L54 and it works fine.

kwhinnery commented 12 years ago

FWIW, module.exports is not in the CommonJS specification at all, although it is a common deviation from the spec. A pure implementation of the spec, arguably, would allow only the assignment of properties to the exports object, not direct assignment to it. While it is cumbersome, the following:

function MyObject() {
  this.awesome = true;
}
exports = MyObject;

//..... usage.....
var MyObject = require('MyObject');

Is not valid within the specification or associated tests/examples. Mozilla Rhino's CommonJS support works about the same way as Titanium in this sense as well, so the change would be helpful in that environment as well. I have not tested this in node.js and can't comment as to the effectiveness there.

wjcrowcroft commented 12 years ago

Hey guys, thanks for clearing that up. I'll test it out in nodeJS today and make sure it works. My feeling is that if it's good enough for underscore, that's a pretty good act to follow.

euforic commented 12 years ago

I tested on node worked for me. Any luck ?

wjcrowcroft commented 12 years ago

Hey, sorry about the delay - been really busy the past two weeks and will be for another week or so.

Thanks again for the submission and I'll see about merging it in as soon as I have some spare time.

euforic commented 12 years ago

Sounds good thanks!

euforic commented 12 years ago

Any progress on this ?

wjcrowcroft commented 12 years ago

Hey, thanks for the bump and apologies for taking so long to get it merged in.

FYI I simplified it a tiny bit based on underscore.js' method, which seems to work, but please let me know if it's not working like you expected, I might have missed something!

Thanks again.