felixge / node-sandboxed-module

A sandboxed node.js module loader that lets you inject dependencies into your modules.
MIT License
342 stars 42 forks source link

Implements Istanbul coverage support and user-defined sourceTransformers #26

Closed searls closed 10 years ago

searls commented 10 years ago

This PR addresses #25. Three changes in this PR:

  1. CoffeeScript support is abstracted into a new object called a sourceTransformer as opposed to being hard-coded with the _getCompileInfo() method
  2. Istanbul support for code coverage is added using the same means.
  3. Arbitrary user-defined source transformers may be configured with a sourceTransformers option passed to SandboxedModule.require.

I was sure to add unit tests to cover everything I did. I tried to write everything I added in a manner that was congruent with the existing code.

Suggestions:

davemo commented 10 years ago

:+1:

domenic commented 10 years ago

This looks very good. I only have the one comment, about how making Istanbul a new default might not be great, and also that the defaults should be configurable project-wide (so e.g. people who want to use, say, traceur, should be able to add a traceur source transformer once instead of on every require).

Also, since I am no longer actively using this module, if @felixge is OK with it, I think it'd be cool to add you as a maintainer on GitHub and npm, continuing the chain of custodion-ship transfer that started when @felixge handed this off to me :).

felixge commented 10 years ago

This looks very good. I only have the one comment, about how making Istanbul a new default might not be great, and also that the defaults should be configurable project-wide (so e.g. people who want to use, say, traceur, should be able to add a traceur source transformer once instead of on every require).

I agree with Dominic, but since I'm not actively maintaining this module right now, I'm not feeling strongly about it ; ).

Also, since I am no longer actively using this module, if @felixge is OK with it, I think it'd be cool to add you as a maintainer on GitHub and npm, continuing the chain of custodion-ship transfer that started when @felixge handed this off to me :).

Done. @searls you now have github / npm push access! Feel free to merge this patch once you're happy with it.

searls commented 10 years ago

Thanks @felixge & @domenic. I'm going to take a look at implementing two pieces of your feedback before merging this.

  1. Allow this new option (and possibly any option) to be set globally instead of only on each-and-every-require
  2. Make Istanbul (and presumably any new interceptors beyond CoffeeScript) opt-in. Figuring out a scheme that's both obvious and simple may be challenging (for instance, a plugin module system would be a little heavyweight, especially since we don't need to pull in the istanbul dependency in order to provide this support).
searls commented 10 years ago

Okay, I've made both those changes. Hopefully being able to set an app-wide global or require() stubbing will be of use to people even if the sourceTransformers & istsanbul stuff doesn't serve them. :smile:

I'll merge and push a version, bumping minor.

searls commented 10 years ago

Ping @felixge -- it looks like I don't have Github Push access to this repo, after all :)

felixge commented 10 years ago

@searls that's weird, it seems like GitHub forgot that I added you. Did you get an e-mail you were added? Anyway, I just added you again, should work now.

searls commented 10 years ago

Thanks folks. Released as 0.3.0.

felixge commented 10 years ago

:sparkling_heart: thx!