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

chore: transformers must return objects, not strings #10773

Closed
wants to merge 1 commit into from

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Nov 3, 2020

Summary

Follow-up to #10753 with more scheduled breaking changes in the transformer.

Test plan

Green CI

@ahnpnl
Copy link
Contributor

ahnpnl commented Nov 3, 2020

oops !!!! I just made a transformer for jest-preset-angular to return a string 😁

@SimenB
Copy link
Member Author

SimenB commented Nov 3, 2020

Yeah, I'm not 100% convinced we should do this. There's not that much code saved - we can normalize where we call process and be done with it. It's useful for transforms even though - if they have a source map - it'd be better if they returned it as separate objects

@SimenB
Copy link
Member Author

SimenB commented Nov 3, 2020

@thymikee @jeysal thoughts?

@thymikee
Copy link
Collaborator

thymikee commented Nov 3, 2020

I'd keep the normalization for one more version and output a deprecation warning. Should give maintainers some time to migrate.

@SimenB
Copy link
Member Author

SimenB commented Nov 3, 2020

Normalization is const transformed = typeof output === string ? { code: output } : output, so it's not that much code. We could still cleanup internal types so it only happens in one place

@jeysal
Copy link
Contributor

jeysal commented Nov 3, 2020

Yeah TBH 'overloading' the return with string and object doesn't seem bad to me as a permanent solution if we treat it like config by normalizing straight away when we received the result. Unless we want to encourage people in general to have their transformers add some of the optional properties because it improves user experience? In that case an object where people will wonder what other properties they could add would be a good nudge.

@SimenB
Copy link
Member Author

SimenB commented Nov 3, 2020

Unless we want to encourage people in general to have their transformers add some of the optional properties because it improves user experience? In that case an object where people will wonder what other properties they could add would be a good nudge

One obvious one is module format (#9860 (comment)) and it is very much preferable if people return source maps if they have them as json already instead of us extracting them (via a regexp) from the returned string.

Printing some warnings if either map is missing (perhaps they should explicitly say null) or moduleFormat (so they explicitly say CJS) might make sense. Dunno?

@jeysal
Copy link
Contributor

jeysal commented Nov 3, 2020

Yeah in that case a string would always generate a warning anyway so we might as well use that as the deprecation message 'transformers need to specify a moduleFormat from Jest X on, return an object with code and moduleFormat' and go with the gradual migration approach

@ahnpnl
Copy link
Contributor

ahnpnl commented Nov 4, 2020

just an idea related to this type of breaking change: perhaps Jest can start pushing out pre-release major versions as well as update CHANGELOG for pre-release major versions. Also, taking into account of pre-release major versions as a part of release process. I see most of big repos do that so I don't see any reasons not doing it for Jest.

That will give users more possibilities to test changes, adopt anything possible etc.. which is especially useful when coming to major versions with breaking changes. For minor/patch versions, that is not needed.

@SimenB
Copy link
Member Author

SimenB commented Nov 4, 2020

27 will be quite breaking across the board (API wise and changing default, plus underlying implementations), so we'll definitely be pushing out pre-releases for this one 👍 Will start this week or next (a bit delayed due to various nugs that needed patches, but I think we're good to go now)

@ahnpnl
Copy link
Contributor

ahnpnl commented Dec 9, 2020

Do we expect to land this for 27 ?

@SimenB
Copy link
Member Author

SimenB commented May 20, 2021

I don't think we need to do this one, did #10823 as solution for "this file is ESM"

@SimenB SimenB closed this May 20, 2021
@SimenB SimenB deleted the transformer-return-object branch May 20, 2021 13:35
@github-actions
Copy link

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 Jun 20, 2021
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.

5 participants