jstransformers / jstransformer-lodash

Lodash support for JSTransformers.
http://npm.im/jstransformer-lodash
MIT License
0 stars 0 forks source link

Use _.runInContext #2

Open jdalton opened 9 years ago

jdalton commented 9 years ago

I noticed you have:

lodash = require('lodash');

then later

lodash.mixin(...);

You could use _.runInContext.

lodash = require('lodash').runinContext();

To avoid potential conflicts caused by augmenting the lodash package directly.

ForbesLindesay commented 9 years ago

:+1:

It looks like we could also replace:


    Object.keys(options.mixins).forEach(function(key) {
      var value = options.mixins[key];
      var mixin = {};
      mixin[key] = value;

      lodash.mixin(mixin);
    })

with

    Object.keys(options.mixins).forEach(function(key) {
      lodash[key] = options.mixins[key];
    })

which is much simpler, or alternatively:

lodash.mixin(options.mixins);
tunnckoCore commented 9 years ago

hmm... strange, didn't notify me.

Okey, so we can do that?

  options = options || {}
  if (options.mixins) {
    lodash.mixin(options.mixins)
  }
  options = lodash.omit(options, ['mixins']);
  var compile = lodash.template(str, options);
  var context = lodash.omit(options, ['imports']);
tunnckoCore commented 9 years ago

hmm.. doh, I dont know, soon I'll get back to jstransformer.

RobLoach commented 9 years ago

As an alternative of requiring the whole lodash package: https://github.com/jstransformers/jstransformer-lodash/pull/3

var lodash = {
  template: require('lodash.template'),
  mixin: require('lodash.mixin'),
  assign: require('lodash.assign'),
  omit: require('lodash.omit')
};
TimothyGu commented 9 years ago

Superseded by #3.

ForbesLindesay commented 9 years ago

We should still create a new instance whenever mixing are provided or they can leak from one call to another.