Sourcemaps #3458
Sourcemaps #3458
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks! If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact [email protected] if you have any questions. |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
|
@cpojer any updates on this one? Can I help somehow? |
|
@benedyktdryl I haven't had time to go back to this, but you could investigate why the CircleCI and AppVeyor builds failed. Also the PR needs to be rebased, so not sure how painful that would be. |
| .install({ | ||
| handleUncaughtExceptions: false, | ||
| retrieveSourceMap: source => { | ||
| if (runtime._sourceMapRegistry[source]) { |
There was a problem hiding this comment.
I don't think accessing a private field of runtime should be done here :)
|
The overall approach looks good, but it needs a rebase and tests like mentioned in the initial PR. |
|
Perspectives on performance: We load source-map support in ts-jest for every test, and after fixing some caching issues we're not that much slower than regular jest. |
|
Right, but this is a feature that most people don't really need because |
|
fwiw I strongly dispute "most people don't really need"; not every babel plugin supports that setting nor is capable of retaining line numbers, and correct source maps are something that people generally seem to universally need. |
All these performance improvements were obtained by using source-map-support's caching. A test repo that we were examining showed very little difference between using pure jest (with javascript) compared to the typescript version using ts-jest |
|
Worth mentioning is, we used sourcemap-support in both those versions, but one version was without source-map-supports caching. |
|
Cool! Would you mind rebasing and making CI green before we can merge this? |
|
Ping! It would be great to get this into Jest now, but it requires a rebase :) |
Under the new transformer rules, if the transformer provides us a sourcemap, we'll take it, regardless of the coverage settings. This means source maps are cached alongside the originals, and `Runtime` will populate its `_sourceMapRegistry`
felipeochoa
force-pushed
the
sourcemaps
branch
from
ee757f9
to
db0cb26
Compare
felipeochoa
force-pushed
the
sourcemaps
branch
from
db0cb26
to
bc671c7
Compare
It was already included as a transitive dependency: babel-core -> babel-register -> source-map-support
package.json
Outdated
| @@ -73,6 +73,7 @@ | |||
| "rollup-plugin-node-globals": "^1.1.0", | |||
| "rollup-plugin-node-resolve": "^3.0.0", | |||
| "slash": "^1.0.0", | |||
| "source-map-support": "^0.5.0", | |||
There was a problem hiding this comment.
Why is this here? Shouldn't it be a dependency of jest-jasmine2?
There was a problem hiding this comment.
You're right. I just moved it there
|
I ran the test suite locally, and on my machine the failing test matches the failing test on circle, so I just updated it and pushed. Hopefully stuff is green 🙂 ProTip:tm: if you have a failing test suite which passes on Jest's CI is to do |
|
There are now less failures on CI, it just seems to be a hash that fails. I'm not sure how that hash is calculated? If it's machine specific it should not be included in the snapshot. Seeing as the same test fails on circle and travis, but with different hashes there, |
|
Yap that makes sense @SimenB. |
Codecov Report
@@ Coverage Diff @@
## master #3458 +/- ##
==========================================
- Coverage 56.17% 55.64% -0.54%
==========================================
Files 194 186 -8
Lines 6542 6351 -191
Branches 3 3
==========================================
- Hits 3675 3534 -141
+ Misses 2866 2817 -49
+ Partials 1 0 -1Continue to review full report at Codecov.
|
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
This is an initial implementation of source map support (#336). Source maps were already supported for outputting coverage information when using the
mapCoverageoption. This PR uses the same machinery to allow any transformer to provide source maps for the runtime to use when generating stack traces (using thesource-map-supportpackage). With this change, transformers can provide source maps by simply returning a{code, map}object instead of the transformed string.Test plan
There are a couple of items tested:
Runtimesource map cache. See abc1508Pending tasks
source-map-supportis already included as a dependency somewhere in the tree, but I need to bring it up to the topcc @cpojer -- let me know what you think of this!