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

Jump to bottom

fix: append sourceMappingURL comment to transformed code #10534

Closed
wants to merge 5 commits into from
Closed

fix: append sourceMappingURL comment to transformed code #10534

wants to merge 5 commits into from

Conversation

aleclarson
Copy link
Contributor

@aleclarson aleclarson commented Sep 19, 2020 •
edited

Summary

This fixes source mapping by the debugger in modules processed by a transformer.

Test plan

No test plan. Tested manually with success.

SimenB
SimenB approved these changes Sep 19, 2020
View reviewed changes
Copy link
Member

@SimenB SimenB left a 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!

SimenB
SimenB reviewed Sep 19, 2020
View reviewed changes
packages/jest-transform/src/__tests__/__snapshots__/script_transformer.test.js.snap Outdated
@@ -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
Copy link
Member

@SimenB SimenB Sep 19, 2020 •
edited

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?

Copy link
Contributor Author

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.

@aleclarson
Copy link
Contributor Author

Ping @thymikee @jeysal

@jeysal
Copy link
Contributor

jeysal commented Oct 1, 2020

Not sure I understand the context around this. What does this do that inline sourcemaps can't do already?

@aleclarson
Copy link
Contributor Author

@jeysal Jest plugins can return a { code, map } object. When the code string has no sourceMappingURL, Jest needs to append one before Runtime._execModule is reached. The map object/string is cached, so we use its cache path in the sourceMappingURL.

Before this patch, modules transformed by esbuild-jest were not being source-mapped by the debugger. Passing sourcemap: 'inline' to esbuild's buildSync function does not work, since the inline source map will contain an incorrect relative path in the sources array (hence the need for this line).

This patch won't conflict with Jest plugins using inline source maps. I've tested it. 👍

@SimenB
Copy link
Member

SimenB commented Oct 2, 2020

Could you revert the snapshot commit I pushed here? Should solve the CI failure

@aleclarson
Copy link
Contributor Author

LGTM

@SimenB
Copy link
Member

SimenB commented Oct 8, 2020

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

@ycjcl868 ycjcl868 mentioned this pull request Nov 11, 2020
Merged
@airhorns
Copy link
Contributor

airhorns commented Jan 6, 2021

How confident are you folks that it should be jest's responsibility to insert the inline sourcemap, and not the transform's? babel-jest uses the sourceMap: "both" option to insert an inline sourcemap all on its own and also return a sourcemap to hand to Jest. I just opened aelbore/esbuild-jest#16 to do the same for esbuild-jest. Another option would be to just make Jest a client of inline sourcemaps instead of expecting to be handed the sourcemap in the map property. The solution in this PR seems a bit strange though, jest wants you to transform the file, keep the sourcemap separate, and then jest itself will reinsert it into the file?

@aleclarson
Copy link
Contributor Author

@airhorns In this PR, Jest doesn't care how you provide the sourcemap. Both inline and returned sourcemaps are supported.

@airhorns
Copy link
Contributor

airhorns commented Jan 7, 2021

Ah, my bad, didn't realize it supported both.

@airhorns
Copy link
Contributor

airhorns commented Oct 9, 2021

Would be great to get this in!

SimenB
SimenB requested changes Oct 9, 2021
View reviewed changes
Copy link
Member

@SimenB SimenB left a 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

packages/jest-transform/src/ScriptTransformer.ts
@@ -432,6 +432,9 @@ class ScriptTransformer {
map = typeof instrumented === 'string' ? null : instrumented.map;
} else {
code = transformed.code;
if (map) {
Copy link
Member

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?

@github-actions
Copy link

github-actions bot commented Oct 9, 2022

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.

@github-actions github-actions bot added the Stale label Oct 9, 2022
@github-actions
Copy link

github-actions bot commented Nov 8, 2022

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.

@github-actions github-actions bot closed this Nov 8, 2022
@github-actions
Copy link

github-actions bot commented Dec 9, 2022

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@SimenB SimenB SimenB requested changes

@thymikee thymikee Awaiting requested review from thymikee

@jeysal jeysal Awaiting requested review from jeysal

Assignees
No one assigned
Labels
cla signed Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@aleclarson @jeysal @SimenB @airhorns @facebook-github-bot