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

Implements Istanbul coverage support and user-defined sourceTransformers #26

Merged
merged 5 commits into from Oct 20, 2013
Merged

Implements Istanbul coverage support and user-defined sourceTransformers #26

merged 5 commits into from Oct 20, 2013

Conversation

searls
Copy link
Collaborator

@searls searls commented Oct 19, 2013

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:

  • Right now requiring coffeescript and istanbul isn't cached between SandboxedModule.require invocations (and hadn't been previously, either. It may be significantly more performant to memoize those methods.

* For fear that glboal.$$cov... might be present w/o
istanbul, I wrapped the istanbul interceptor in a big
try catch

* To prevent the finding coffeescript.compile and
istanbul instrumentSync on every single file, I 
cache them.
@davemo
Copy link

davemo commented Oct 19, 2013

👍

An object of named functions which will transform the source code required with
`SandboxedModule.require`. For example, CoffeeScript &
[istanbul](https://github.com/gotwarlost/istanbul) support is implemented as
default sourceTransformer functions.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there an interface to set the default source transformers, project-wide? Also, I am not sure including istanbul by default is good; I would actually remove CoffeeScript except for backward-compat.

@domenic
Copy link
Collaborator

domenic commented Oct 19, 2013

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
Copy link
Owner

felixge commented Oct 20, 2013

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
Copy link
Collaborator Author

searls commented Oct 20, 2013

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).

Presumably this will be appropriate for any future
built-in source transformers added to the project.
@searls
Copy link
Collaborator Author

searls commented Oct 20, 2013

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. 😄

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

@searls
Copy link
Collaborator Author

searls commented Oct 20, 2013

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

@felixge
Copy link
Owner

felixge commented Oct 20, 2013

@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 added a commit that referenced this pull request Oct 20, 2013
Implements Istanbul coverage support and user-defined sourceTransformers
@searls searls merged commit 8b6d4fb into felixge:master Oct 20, 2013
@searls
Copy link
Collaborator Author

searls commented Oct 20, 2013

Thanks folks. Released as 0.3.0.

@felixge
Copy link
Owner

felixge commented Oct 20, 2013

💖 thx!

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

Successfully merging this pull request may close these issues.

None yet

4 participants