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
fix: append sourceMappingURL comment to transformed code #10534
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes sense, thanks!
@@ -126,6 +126,7 @@ cov_25u22311x4(); | |||
cov_25u22311x4().s[0]++; | |||
module.exports = "banana"; | |||
//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbImJhbmFuYS5qcyJdLCJuYW1lcyI6WyJtb2R1bGUiLCJleHBvcnRzIl0sIm1hcHBpbmdzIjoiOzs7Ozs7Ozs7Ozs7Ozs7Ozs7Ozs7Ozs7Ozs7Ozs7Ozs7Ozs7Ozs7QUFlWTs7Ozs7Ozs7OztBQWZaQSxNQUFNLENBQUNDLE9BQVAsR0FBaUIsUUFBakIiLCJzb3VyY2VzQ29udGVudCI6WyJtb2R1bGUuZXhwb3J0cyA9IFwiYmFuYW5hXCI7Il19 | |||
//# sourceMappingURL=/cache/jest-transform-cache-test/5b/banana_5bf5b7017ea79e485ba49f739d279573.map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aleclarson both inline and map link here - seems wrong. I'm worried this might cause issues (similar to what they tackled here vuejs/vue-jest#255). Even if "only the first is used", it seems this might cause issues? Should we skip any that already have an inline one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, so the Runtime._instrumentFile
method appends an inline sourcemap. Good to know!
I updated my patch to avoid appending the sourceMappingURL
when code coverage is enabled for a file. That should avoid any conflicts!
I manually tested it, and it works with and without code coverage enabled. 👍
I also tested with a transform that emits an inline sourcemap, and no issues there.
Not sure I understand the context around this. What does this do that inline sourcemaps can't do already? |
@jeysal Jest plugins can return a Before this patch, modules transformed by This patch won't conflict with Jest plugins using inline source maps. I've tested it. 👍 |
Could you revert the snapshot commit I pushed here? Should solve the CI failure |
LGTM |
6b6ce41
to
d8c9883
Compare
Could we check the last line in the source code for an existent link before appending? I'm still a bit leery adding this. I'd also still like to see a unit test so it's not accidentally lost in a future refactor |
This fixes source mapping by the debugger in modules processed by a transformer.
d8c9883
to
cb88672
Compare
How confident are you folks that it should be jest's responsibility to insert the inline sourcemap, and not the transform's? |
@airhorns In this PR, Jest doesn't care how you provide the sourcemap. Both inline and returned sourcemaps are supported. |
Ah, my bad, didn't realize it supported both. |
Would be great to get this in! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left an inline comment. No matter which way that is resolved, this PR needs tests
@@ -432,6 +432,9 @@ class ScriptTransformer { | |||
map = typeof instrumented === 'string' ? null : instrumented.map; | |||
} else { | |||
code = transformed.code; | |||
if (map) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep it in the if (map)
below, but check if the source code has a sourceMappingURL
statement already.
Not sure if we then should replace it with our own or just ignore it?
This PR is stale because it has been open 1 year with no activity. Remove stale label or comment or this will be closed in 30 days. |
This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one. |
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 fixes source mapping by the debugger in modules processed by a transformer.
Test plan
No test plan. Tested manually with success.