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

use capture-require #77

Closed

Conversation

jamestalmage
Copy link
Member

Well, this doesn't work, but I am a bit stumped as to why.

It no longer distinguishes between default and wrapped (it just uses the wrapping function to wrap the current top of require.extensions['.js']). This also means it never calls addFile, but addFile isn't really necessary. The default require.extensions['.js'] should be forwarding you the content anyway.

@novemberborn - can you take a look and give me a guess as to why those tests are failing (like what information nyc isn't collecting correctly). I've spent quite a bit of time groking the wrapRequire section of the nyc codebase, but haven't really dived deep anywhere else.

I will keep trying, just hoping for another set of eyeballs.

@bcoe
Copy link
Member

bcoe commented Dec 7, 2015

@jamestalmage we'll get it to work, I'm really happy with how much logic this will trim out of the core library -- nyc's strength is in it's subprocess handling, I love the idea of splitting more and more logic into other modules.

Complete aside but, I wonder if it's worth pulling the source-map-remapping logic into its own module.... it is pretty domain specific, since it assumes an Istanbul coverage output.

@novemberborn
Copy link
Contributor

@jamestalmage I'm not quite sure. It may be to do with #79, or that the tests tend to reload index.js after clearing it from the require cache.

@jamestalmage
Copy link
Member Author

Complete aside but, I wonder if it's worth pulling the source-map-remapping logic into its own module.... it is pretty domain specific, since it assumes an Istanbul coverage output.

Modularize all the things. 👍

@jamestalmage
Copy link
Member Author

I think part of the problem might be that the old .wrap() just blew away whatever was existing on require.extensions['.js']. That meant repeated calls to .wrap() did not have side effects. In an attempt to be more generalized capture-require actually looks at Object.getOwnPropertyDescriptor and will forward through future added extensions to the previous getter/setter pair. Since nyc installs its hook multiple times in the test, this ends up meaning the coverage transform gets applied multiple times (once for the hook installed in this test, and once for every previous test).

All speculation on my part at this point. But if it turns out to be true, then we actually only have a problem in the tests, since nyc won't invoke capture-require multiple times IRL.

Not sure on the best strategy for solving that.

@novemberborn
Copy link
Contributor

Not sure on the best strategy for solving that.

Let's see if we can tackle #80.

@bcoe
Copy link
Member

bcoe commented Dec 12, 2015

@jamestalmage any headway on this bug?

@jamestalmage
Copy link
Member Author

closed via #90

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.

3 participants