Skip to content

feat: return transformed code as a string, do not wrap in vm.Script#9253

Merged
SimenB merged 13 commits into
jestjs:masterfrom
SimenB:pass-in-module-wrapper-arguments
Dec 1, 2019
Merged

feat: return transformed code as a string, do not wrap in vm.Script#9253
SimenB merged 13 commits into
jestjs:masterfrom
SimenB:pass-in-module-wrapper-arguments

Conversation

@SimenB
Copy link
Copy Markdown
Member

@SimenB SimenB commented Dec 1, 2019 •
edited
Loading

Summary

Extracted from #9252. Since jest-runtime is the one injecting arguments into the module wrapper, it should also say which arguments those are. This makes @jest/transform less coupled to the module wrapper, and thus to Jest's use case. It's marked as a breaking change since it changes @jest/transform's interface, but it's not a breaking change for users of Jest.

Test plan

Added some tests

@SimenB SimenB requested review from jeysal and thymikee December 1, 2019 15:04
SimenB
SimenB commented Dec 1, 2019
View reviewed changes
Comment thread packages/jest-transform/src/types.ts Outdated
@SimenB SimenB mentioned this pull request Dec 1, 2019
Merged
thymikee
thymikee reviewed Dec 1, 2019
View reviewed changes
Comment thread packages/jest-runtime/src/index.ts Outdated
thymikee
thymikee reviewed Dec 1, 2019
View reviewed changes
Comment thread CHANGELOG.md Outdated
@SimenB SimenB force-pushed the pass-in-module-wrapper-arguments branch from ef1a97e to 9289c94 Compare December 1, 2019 16:27
thymikee
thymikee reviewed Dec 1, 2019
View reviewed changes
Comment thread packages/jest-runtime/src/index.ts Outdated
@SimenB
Copy link
Copy Markdown
Member Author

SimenB commented Dec 1, 2019 •
edited
Loading

We could ditch the wrapping in @jest/transform entirely, and instead do that in jest-runtime directly. It's a weird coupling anyways.

EDIT: Currently doing this, and it's way cleaner. Probably something I should have done when @jest/transform was split out from jest-runtime

@SimenB SimenB mentioned this pull request Dec 1, 2019
Merged
@SimenB SimenB force-pushed the pass-in-module-wrapper-arguments branch from 56ebbf7 to 4965e77 Compare December 1, 2019 20:05
@SimenB SimenB changed the title feat: pass in wrapper arguments to transformer feat: return transformed code as a string, do not wrap in vm.Script Dec 1, 2019
@SimenB SimenB requested a review from thymikee December 1, 2019 20:06
@SimenB
Copy link
Copy Markdown
Member Author

SimenB commented Dec 1, 2019 •
edited
Loading

@thymikee I've moved the entire wrapping into jest-runtime, I think it turned out way cleaner. Now @jest/transform does not have to care about the wrapper at all, neither creating it nor passing the correct arguments to it. I've also made jest-environment not care about the wrapper, since it actually doesn't care. It just invokes the script passed to it and returns the result. I'm quite happy with how this turned out

SimenB
SimenB commented Dec 1, 2019
View reviewed changes
Comment thread packages/jest-transform/src/__tests__/script_transformer.test.js
expect(vm.Script.mock.calls[1][0]).toEqual(snapshot);

scriptTransformer.transform('/fruits/banana.js', {
// to make sure jest isn't declared twice
Copy link
Copy Markdown
Member Author

@SimenB SimenB Dec 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was broken anyways - if we declared it and then removed it here, we would still inject it from jest-runtime

thymikee
thymikee approved these changes Dec 1, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely

@SimenB SimenB merged commit 1d8245d into jestjs:master Dec 1, 2019
@SimenB SimenB deleted the pass-in-module-wrapper-arguments branch December 1, 2019 21:45
This was referenced Jan 28, 2020
Open
Open
This was referenced Feb 4, 2020
Open
Open
Open
Open
This was referenced Feb 18, 2020
Open
Open
Open
@Leko Leko mentioned this pull request Feb 25, 2020
Open
This was referenced Mar 3, 2020
Open
Open
This was referenced Mar 10, 2020
Open
Open
Open
Open
@snyk-bot snyk-bot mentioned this pull request Mar 21, 2020
Closed
This was referenced Mar 24, 2020
Open
Open
This was referenced Apr 16, 2020
Open
Open
Merged
Closed
Merged
This was referenced Apr 30, 2020
Closed
Closed
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 11, 2021

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 May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@thymikee thymikee thymikee approved these changes

@jeysal jeysal Awaiting requested review from jeysal

Assignees

No one assigned

Labels

cla signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@SimenB @thymikee @facebook-github-bot