Updated CONTRIBUTING

Plus a few fixed h/t linter
AidenRourke · Dec 13, 2016 · 7eadccb · 7eadccb
1 parent 1ac4aa1
commit 7eadccb
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 18 deletions.
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -1,17 +1,9 @@
-## Contributing
+# Contributing
 
 Thank you for contributing!
 
-The source is organized like this:
 
-```
-/dist       # Distributables
-/build      # Intermediate build files
-/src        # JS source files
-/native     # C source files for curve25519
-/protos     # Protobuf definitions
-/test       # Tests
-```
+## Getting started
 
 To compile curve25519 from C souce files in `/native`, install
 [emscripten](https://kripken.github.io/emscripten-site/docs/getting_started/downloads.html).
@@ -32,6 +24,26 @@ then set the appropriate envirionment variables to run the tests:
 SAUCE_USERNAME="your-sauce-username" SAUCE_ACCESS_KEY="your-sauce-key" grunt test
 ```
 
+## Code structure
+
+The source is organized like this:
+
+```
+/dist       # Distributables
+/build      # Intermediate build files
+/src        # JS source files
+/native     # C source files for curve25519
+/protos     # Protobuf definitions
+/test       # Tests
+```
+
+The main app entrypoint is `src/main.js`. This is what the caller would require in node, or bundle in browserify (though see note below, Node/browser polyfills).
+
+The main test entrypoint is `test/main.js`.
+`test/index.html` compiles everything we need for our tests in the browser.
+
+You can run these by serving the project root and visiting /test (e.g. http://localhost:8000/test).
+
 
 ## Node/browser polyfills
 
@@ -41,3 +53,18 @@ Just search the source for:
 ```js
 // BROWSER POLYFILL
 ```
+
+to see where they are used.
+
+The file with *all* the polyfills is `src/node_polyfills.js`
+
+We ignore this file when we do our browserify build for testing.
+
+**NOTE/QUESTION**.
+Ignoring the polyfill in our test browserify build certainly makes the tests pass, but a caller who downloads the stuff from npm, tries to `require` our package and browserify it for themelves, will be in for a nasty surprise. Lots of native gunk will show up in their browserify errors, and they will probably be confused and walk away. So, how should we make the browser-side require more seamless? Can we have people require a browserified build we produce?
+
+
+## TODO
+- Integrate native test into grunt test routine
+- Travis builds for node?
+- Fix native bundling issue, mentioned above
diff --git a/Gruntfile.js b/Gruntfile.js
@@ -1,6 +1,11 @@
 var child_process = require('child_process');
 var util = require('util');
 
+// browserify should ignore the native polyfills
+var browserifyOpts = {
+  exclude: ['src/node_polyfills.js'],
+};
+
 module.exports = function(grunt) {
   'use strict';
 
@@ -10,16 +15,12 @@ module.exports = function(grunt) {
       legacy: {
         src: 'src/main_window.js',
         dest: 'dist/libsignal.js',
-        options: {
-          exclude: ['src/node_polyfills.js'],
-        }
+        options: browserifyOpts
       },
       test: {
         src: 'test/main.js',
         dest: 'build/test_main.js',
-        options: {
-          exclude: ['src/node_polyfills.js'],
-        }
+        options: browserifyOpts
       }
     },
     concat: {

diff --git a/src/curve25519_worker_manager.js b/src/curve25519_worker_manager.js
@@ -12,7 +12,7 @@ function workerRoutine () {
       self.postMessage({ id: e.data.id, error: error.message });
     });
   };
-};
+}
 
 
 function Curve25519Worker() {
@@ -22,7 +22,7 @@ function Curve25519Worker() {
   try {
     var work = require('webworkify');
     this.worker = work(function (self) {
-      workerRoutine()
+      workerRoutine();
     });
   } catch (e) {
     var Worker  = require('./node_polyfills.js').Worker;