Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Need a better module wrapping #66

Closed
6 tasks
dchest opened this issue Jan 12, 2015 · 13 comments · Fixed by #87
Closed
6 tasks

Need a better module wrapping #66

dchest opened this issue Jan 12, 2015 · 13 comments · Fixed by #87

Comments

@dchest
Copy link
Owner

dchest commented Jan 12, 2015

We need something like UMD for module wrapping with these features:

  • Global nacl/window.nacl.
  • CommonJS:
    • Node.js, with PRNG using require('crypto')
    • Browserify/Webpack using WebCrypto, not require('crypto') emulation (without this workaround).
  • AMD
  • root object inside the module, which can be null or undefined for Node, window for browsers, self (?) for webworkers in browsers (to fix Way to use nacl in web workers #65)
@jo
Copy link

jo commented Apr 11, 2015

I suggest to use browserify for this:

This solves all the requirements stated above (it also considers self), except AMD support.
Its the route we took successfully with PouchDB.

@dchest
Copy link
Owner Author

dchest commented Apr 13, 2015

Thanks, I'll think about it, but I don't want to introduce another build tool just for a few lines of code.

@jo
Copy link

jo commented Apr 13, 2015

I totally understand your objections. Just to add: since here is already a build step (uglifyjs) it does not add more maintenance efford. And browserify has been made exactly for this purpose.

@dchest
Copy link
Owner Author

dchest commented Apr 13, 2015

@RainaBatwing do you think it will solve #59?

@jo where can I take a look at what browserify replaces Buffer with? Thanks!

@jo
Copy link

jo commented Apr 13, 2015

@dchest
Copy link
Owner Author

dchest commented Apr 13, 2015

@jo thank you! It's kinda big, but would solve #44, #38, and #59. Unfortunately, it's using MIT license, and I'd like to keep TweetNaCl public domain. Maybe I'll write my own? :-) Does browserify allows custom replacements for Node objects?

@jo
Copy link

jo commented Apr 13, 2015

Yes, I think in package.json you can just point to a file or (a node module):

"browser": {
  "buffer": "./lib/buffer.js"
}

https://github.com/substack/node-browserify#browser-field

Extensive docu can be found in the Browserify Handbook.

@RainaBatwing
Copy link

I would like to see this change, but I'd much rather you use the browserify Buffer than invent a new one. MIT license is not a restrictive one, and having more code that can potentially introduce bugs seems antithetical to the spirit of tweetnacl as a small audit-able library.

If there really are users who cannot use MIT-licensed software but can accept third party public domain code, they could open an issue later and we can reconsider then?

@dchest
Copy link
Owner Author

dchest commented May 13, 2015

Thanks for opinions, everyone. I'm sorry that I'm so picky about this change, but I just can't handle this little fact:

  • Buffer.js — 38 KB
  • nacl.js — 33 KB

I know, an average image on a website is bigger than that, but still I'd like to avoid including a library for frigging byte arrays that's bigger than the whole project. Especially, since Buffer is here mostly for Node.js, which is better served by a native NaCl.

@alax
Copy link

alax commented May 13, 2015

So it seems like the arguments are:

Node Buffers (and browserify shim):

  • One code base to audit for buffers
  • In theory, better audited since it's more widely used
  • More consistent buffer implementations
  • Solves a few outlying bugs

TweetNaCl typed arrays:

  • Typed arrays are native to the browser
  • Small, concise code that should be easy to audit

Personally, I'd say stick with the typed arrays, since it does make more sense in the browser. TweetNaCl is great for the client-side, and if you'd like to run it under Node, that's easily doable even with the typed arrays. But, like @dchest mentioned, under Node you really could just use a library with native NaCl bindings, which would definitely offer better performance, and would probably use buffers as well.

So, +1 for the current implementation from me. Small and simple on the client-side, but still fairly simple to use on the server-side if necessary.

@dchest
Copy link
Owner Author

dchest commented May 13, 2015

Another option is to implement both Buffers and typed arrays as @RainaBatwing proposed in #59. If you pass a Buffer to functions, they will return Buffers, if you pass a typed array, they will return typed arrays.

@RainaBatwing
Copy link

My interest in using tweetnacl-js is because I'm developing cross platform p2p apps using NW.js and depending on platform-specific code makes software update distribution much more complicated. For my things, there are no clients or servers. I'd like to have just one version which works on every platform, and I'd like all the application specific code to be in javascript, so anyone can take it and alter it easily. I strongly dislike the idea of shipping binary packages, but most users do not have C build tools on their computer.

dchest added a commit that referenced this issue Feb 19, 2016
randombytes in browsers no longer throw when output length exceeds 65536
bytes. Remove mention of this from README.

Improve detection of environment (browser or Node) by also checking
process.browser.

Replace references to window with self. Fixes #65, closes #66.
dchest added a commit that referenced this issue Feb 19, 2016
randombytes in browsers no longer throw when output length exceeds 65536
bytes. Remove mention of this from README.

Improve detection of environment (browser or Node) by also checking
process.browser.

Replace references to window with self. Fixes #65, closes #66.
dchest added a commit that referenced this issue Feb 19, 2016
randombytes in browsers no longer throw when output length exceeds 65536
bytes. Remove mention of this from README.

Improve detection of environment (browser or Node) by also checking
process.browser.

Replace references to window with self. Fixes #65, closes #66.
dchest added a commit that referenced this issue Feb 20, 2016
randombytes in browsers no longer throw when output length exceeds 65536
bytes. Remove mention of this from README.

Improve detection of environment (browser or Node) by also checking
process.browser.

Replace references to window with self. Fixes #65, closes #66.
@tarun1475
Copy link

Use react-native-tweetnacl npm package for using tweetnacl in mobile apps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants