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

Globals and exports #3

Open
mryellow opened this issue Sep 18, 2015 · 6 comments
Open

Globals and exports #3

mryellow opened this issue Sep 18, 2015 · 6 comments

Comments

@mryellow
Copy link

Any reason for deciding to go with globals over checking for browser and deliveringwindow.RL or module.exports.RL?

Something you'd accept as a PR, if it was done in a way that fits?

@karpathy
Copy link
Owner

Hi, there is no reason for this except that I don't know how it should be done properly. If you can fix this with a PR I'd be happy to merge

@karpathy
Copy link
Owner

is this a big change? I'm not exactly sure what's involved. And is this done so that things work transparently on both node and browser? Or what's wrong with the current way? It does create two new globals R and RL that hold the lib.

@mryellow
Copy link
Author

how it should be done properly

Looks like that check pattern you included on the ConvnetJS which dumps it into window.foo or module.exports.foo is best practice. Not something I've done a million times either....

is this done so that things work transparently on both node and browser?

I believe that check takes care of all the cases. Anything which would require global instead of window?

Now what is wrong with the current way..... Maybe nothing....

You're keeping everything simple and flat without package managers, maybe the best bet is a separate wrapper which can be used with node, which simply does the checks and throws everything in exports regardless.

@mryellow
Copy link
Author

mryellow commented Oct 1, 2015

I've wrapped it using this pattern, once Math.tanh is polyfilled for NodeJS v0.10.40 everything works as expected.

execfile.js

var vm = require("vm");
var fs = require("fs");
module.exports = function(path, context) {
  context = context || {};
  var data = fs.readFileSync(path);
  vm.runInNewContext(data, context, path);
  return context;
};

foo.js

var execfile = require("./execfile");

// Polyfill ES6.
// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/tanh
Math.tanh = Math.tanh || function(x) {
  if (x === Infinity) {
    return 1;
  } else if (x === -Infinity) {
    return -1;
  } else {
    var y = Math.exp(2 * x);
    return (y - 1) / (y + 1);
  }
};

var RL = execfile('./lib/rl.js', {
  Math: Math // Not certain this was needed...
}).RL;

@Rob-Voss
Copy link

Rob-Voss commented Oct 5, 2015

@mryellow
Copy link
Author

mryellow commented Oct 5, 2015

Yeah similar to this:

https://github.com/karpathy/convnetjs/blob/master/src/convnet_export.js

Think in the end it's probably not really needed, depends on if this lib is about being a block of code in one file, simple and with much of the bones showing... or a package for people to install and run with.

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

No branches or pull requests

3 participants