Don't use relative AMD module paths and make explicit bigint's dependency on crypto.

[jcbrand]

Hi @arlolra

Thank you for this excellent library. I'm using it to provide OTR functionality in converse.js.
I would appreciate it if you mentioned converse.js under the list of programs using OTR.js

This pull request contains two changes.

• Make explicit bigint's dependency on crypto (See line 5 in bigint.js)

This is the most important change for me. I'm using PhantomJS/Jasmine for tests and without this change they break because root.crypto is undefined.

• Don't use relative paths for AMD modules.

I'm not an AMD/require.js expert but it appears to me that using relative paths to AMD modules in the define call is an anti-pattern.

The reason for this is that converse.js is now forced to include OTR.js also via a relative path and this makes it difficult for consumers of converse.js to integrate it into their AMD/require.js projects, because they now need to maintain the same filesystem layout expected by both converse.js and OTR.js.

Instead, how I think it needs to be done is that require.js's config option must be used to configure the relative paths, and then in the define call only the module name is used. Here is for example how converse.js configures the paths: https://github.com/jcbrand/converse.js/blob/master/main.js

Link to require.js's docs on config: http://requirejs.org/docs/api.html#config

The require.js config call is separate from any modules and is project specific. This means that integrators can configure the paths specifically for their own project and don't need to maintain a specific filesystem structure.

Thanks
JC

[arlolra]

I would appreciate it if you mentioned converse.js under the list of programs using OTR.js

Done in 490af88.

Don't use relative paths for AMD modules.

Much agreed that this probably isn't canonical AMD support. Happy to land that part of the patch. Thanks.

Make explicit bigint's dependency on crypto.

PhantomJS has two problems here,

-    define(['./salsa20'], factory.bind(root, root.crypto))
+    define(['crypto', 'salsa20'], function (crypto, salsa) {
+      return factory.call(root, root.crypto, salsa)
+    });

First, you need to polyfill Function.prototype.bind because, until 2.0, it doesn't support it.

Second, bigint doesn't actually have a dependency on crypto. The crypto here isn't CryptoJS, it's window.crypto.getRandomValues.

Also, in your diff, you still have root.crypto, which would be wrong and you're editing a file in build/ instead of vendor/, but I can see how that 's not entirely clear. Anyways, don't worry about that stuff.

What I think you want to do there is again apply a polyfill. For the sake of your testing, you probably don't care about cryptographic security so something like,

window.crypto = {
  getRandomValues: function (buf) {
    for (var i = 0, len = buf.length; i < len; i++) {
      buf[i] = Math.floor(Math.random() * 256);
    } 
  }
}

but please don't use that in production.

I'll get the relative path changes merged shortly. Thanks again and let me know if that's clear.

[arlolra]

Ok, merged your relative path changes in a484ba4.

The contents in build/ only gets updated when I make a release. Until then you can make build to try it out. Hope that works for you. And, let me know about window.crypto above. Thanks.

[jcbrand]

Great! Thanks for the merge.

You're right, I don't test the crypto stuff so a Window.crypto mock should do the trick. Thanks for the explanation.

[arlolra]

Ok, let me know if you have any problems. Closing.