add vm.runInThisContext hook and automatically call it on run-with-cover for RequireJS support #26

Merged
merged 2 commits into from Jan 3, 2013

Conversation

Projects
None yet
2 participants
Contributor

millermedeiros commented Dec 18, 2012

added failing test on a separate commit and then implemented feature to make sure it is working as expected. Feel free to squash commits / rephrase.

see issue #23 for more info.

Owner

gotwarlost commented Dec 18, 2012

Changes look good to me. Thanks for the pull request. The only thing I'm debating in my mind is if it should be done by default since that is potentially a breaking change for consumers.

I do see the point that this is probably how istanbul should have behaved in the first place.

Also, I need to deal with the flakiness of a couple of tests in the travis build but that is my problem :)

Owner

gotwarlost commented Dec 18, 2012

This may have the potential to break the istanbul hacks around the YUI loader since I believe it uses runInThisContext internally and might cause double-hooking in that case. Give me this weekend to look at the implications and I will merge these in with appropriate safeguards for that case, if required.

Contributor

millermedeiros commented Dec 19, 2012

I would be ok with a flag to toggle the behavior or a simple way to keep this logic on an external file if needed - I'm currently running istanbul test as my npm test script.

Owner

gotwarlost commented Dec 19, 2012

I don't think we need an external file for this. The functionality is generally useful. I think at this point I'm leaning towards a flag to allow users to opt-in into this functionality.

Owner

gotwarlost commented Dec 21, 2012

@millermedeiros I assume you are not blocked in any way until I merge this feature?

Thing is, I have a lot of stuff to finish by year-end so I would like to merge this in early Jan. Will that work for you?

Also, thinking a bit about this, it may be dangerous to hook runInThisContext automatically since, unlike require you can pass an arbitrary dynamically generated script to the call and give it a fake filename that doesn't actually exist. If istanbul covers this case it will blow up in the report generation phase since it won't be able to find this "file".

From that viewpoint, I think it is safer to make this feature an opt-in rather than default behavior so that the user makes a conscious choice.

Contributor

millermedeiros commented Dec 21, 2012

not blocking me in any way, I'm using my fork as a dependency:

  "devDependencies": {
    "istanbul": "https://github.com/millermedeiros/istanbul/tarball/latest"
  }

I'm totally fine with a flag or any other way to toggle the behavior.

in case you want to check on a real project amd-utils uses RequireJS to run the specs on node.js.

gotwarlost added a commit that referenced this pull request Jan 3, 2013

Merge pull request #26 from millermedeiros/master
add vm.runInThisContext hook and automatically call it on run-with-cover for RequireJS support.

Going to merge this as-is and then commit a change to only call this hook if a command line option is passed in.

@gotwarlost gotwarlost merged commit 2693413 into gotwarlost:master Jan 3, 2013

1 check failed

default The Travis build failed
Details
Owner

gotwarlost commented Jan 4, 2013

Added a switch called --hook-run-in-context to the cover and test commands.

  --hook-run-in-context
          hook vm.runInThisContext in addition to require (supports
          RequireJS), defaults to false

Fix available in v0.1.27. Please let me know if it works for you.

Contributor

millermedeiros commented Jan 4, 2013

working great! thanks a lot! updated amd-utils to use it millermedeiros/amd-utils@17cac41 and https://travis-ci.org/millermedeiros/amd-utils/builds/3950150

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment