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

hotfix: inline source maps #655

Closed

Conversation

jamestalmage
Copy link
Contributor

Wait to merge - this might not be the best way to fix the issue.

This fixes an nyc compatibility issue that surfaced here

I am not entirely sure why source-map inlining is required (it worked fine without it in previous versions of AVA).

@novemberborn - It looks like the type of value we return from retrieveSourceMap inside test-worker.js changed from an object to a string sometime between 0.9.2 and 0.13 - I wonder if that has something to do with it.

This fixes an `nyc` compatibility issue that [surfaced here](kentcdodds/react-ava-workshop#5)

I am not entirely sure why source-map inlining is required (it worked fine without it in previous versions of AVA).

@novemberborn - It looks like the type of value we return from `retrieveSourceMap` inside `test-worker.js` changed from an object to a string sometime between `0.9.2` and `0.13` - I wonder if that has something to do with it.
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @spudly to be a potential reviewer

@novemberborn
Copy link
Member

It looks like the type of value we return from retrieveSourceMap inside test-worker.js changed from an object to a string sometime between 0.9.2 and 0.13 - I wonder if that has something to do with it.

Yea I think so. We should try to JSON.parse() the file.

I am not entirely sure why source-map inlining is required (it worked fine without it in previous versions of AVA).

IIRC some of the source map tools support map strings and objects, but others are more picky, or something like that. It might be that the retrieveSourceMap() result is being disregarded and the fallback is to look at an inline source map (or perhaps retrieveSourceMap() is the fallback for when there is no inline source map).

@novemberborn
Copy link
Member

Oh wait got confused between test files, source files, and the source maps used by nyc.

The source map handling in AVA is so we can correct stack traces for errors coming out of the test files. Presumably this works fine even if retrieveSourceMap() returns a map string (due to the aforementioned liberal acceptance of JSON strings and objects).

I suspect nyc is trying to generate coverage for the test files but can't find the source maps. Setting it to inline here might be rectifying that. I'll have to do some more digging to figure out how and why it's instrumenting the test files though, that shouldn't be necessary. I'm not clear at all on how it can even access the transpiled code.

Changing the mapFile extension to .js.map may also help.

@jamestalmage
Copy link
Contributor Author

Changing the mapFile extension to .js.map may also help.

It might if we weren't lying to the module system about the sources location (it's coming from the cache folder, not the modules source path). This is why we have to use a custom function for sourcemap support.

@jamestalmage
Copy link
Contributor Author

Closing in favor of #662

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