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

feat(@jest/transform)!: require process() and processAsync() methods to always return structured data #12638

Merged
merged 12 commits into from
Apr 6, 2022
Merged

feat(@jest/transform)!: require process() and processAsync() methods to always return structured data #12638

merged 12 commits into from
Apr 6, 2022

Conversation

mrazauskas
Copy link
Contributor

Summary

Fixing just another TODO. Transformer methods process and processAsync currently are allowed to return a string or an object. As the comment states, perhaps it is fine to require to always return structured data:

https://github.com/facebook/jest/blob/72ae96aed9865e7f24e6db89024ca7b04fe62cb9/packages/jest-transform/src/types.ts#L34-L37

Test plan

Code change was minimal, but I had to refactor lots of tests. All should pass.

@mrazauskas mrazauskas changed the title feat(@jest/transform)!: require process and processAsync to always return structured data feat(@jest/transform)!: require process() and processAsync() methods to always return structured data Apr 6, 2022
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.

changelog entry? 😀 and https://github.com/facebook/jest/blob/main/docs/CodeTransformation.md needs some updates

EDIT: hah
image

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.

yay!

@mrazauskas mrazauskas marked this pull request as draft April 6, 2022 09:16
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.

wait, RN is failing. might need to patch it (merge in main first, I just updated it to v0.68)

@SimenB
Copy link
Member

SimenB commented Apr 6, 2022

can use this patch: facebook/react-native#33576

@mrazauskas
Copy link
Contributor Author

mrazauskas commented Apr 6, 2022

can use this patch: facebook/react-native#33576

Great. I will try.

Docs looked dated. Just could not stop myself. If that’s too much, I can open separate PR.

@mrazauskas mrazauskas marked this pull request as ready for review April 6, 2022 10:34
@mrazauskas mrazauskas requested a review from SimenB April 6, 2022 10:34
@@ -1,8 +1,17 @@
diff --git a/jest/preprocessor.js b/jest/preprocessor.js
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yarn placed this file inside .yarn directory. Perhaps a good idea?

Copy link
Member

Choose a reason for hiding this comment

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

yep, this is where it should be (new in yarn v3 there's a built in --save)

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.

great stuff!

@github-actions
Copy link

github-actions bot commented May 7, 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 May 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants