feat: extract 'createTransformer' and use type predicates

[fatso83]

Summary

See discussion in #12403. These changes make the interfaces more self-documenting and the intent clearer.

Test plan

Type checks

$ npm run test-types

> @jest/[email protected] test-types /home/carlerik/code/jest
> yarn jest --config jest.config.tsd.js

 PASS   types  packages/jest-types/__typetests__/config.test.ts
 PASS   types  packages/jest-types/__typetests__/jest.test.ts
 PASS   types  packages/expect/__typetests__/expect.test.ts
 PASS   types  packages/jest-types/__typetests__/globals.test.ts
 PASS   types  packages/jest-types/__typetests__/expect.test.ts

Test Suites: 5 passed, 5 total
Tests:       412 passed, 412 total
Snapshots:   0 total
Time:        4.334 s
Ran all test suites.

Unit tests
This failed on some integration tests (differed on runs), but not due to my changes?

$ npm run test

> @jest/[email protected] test /home/carlerik/code/jest
> yarn lint && yarn jest

Summary of all failing tests
 FAIL  e2e/__tests__/complexItemsInErrors.ts (6.038 s)
  ● handles functions that close over outside variables

    Timed out

      at timeoutKill (node_modules/execa/lib/kill.js:65:23)

  ● handles functions that close over outside variables

    Process exited

      51 |   );
      52 |
    > 53 |   await waitUntil(({stderr}) => stderr.includes('Ran all test suites.'));
         |         ^
      54 |
      55 |   const {stderr} = await end();
      56 |

      at Object.waitUntil (e2e/__tests__/complexItemsInErrors.ts:53:9)


Test Suites: 1 failed, 1 skipped, 426 passed, 427 of 428 total
Tests:       1 failed, 38 skipped, 4515 passed, 4554 total
Snapshots:   1748 passed, 1748 total
Time:        320.018 s

[SimenB]

yay!

[fatso83]

Hmm ... after seeing babel-jest breaking due to a type issue, I tried fixing it, but I am having difficulty getting it to recognize that the type is indeed exported. When running npm run watch:ts I get the same error as the CI:

packages/babel-jest/src/index.ts:22:3 - error TS2305: Module '"@jest/transform"' has no exported member 'TransformerCreator'.

The transform package does (now) have that type exported and I can see it in node_modules/\@jest/transform/build/types.d.ts, so not sure what is going on here. Hoping that pushing a commit fixes 🤷 Seems to be some kind of build ordering issue.

[SimenB]

Note that you need to export TransformerCreator from index.ts for it to be visible outside of the module it's declared in

[fatso83]

Finally builds every (*) integration test 🥳 Trang fødsel, as we say. This "simple doc update" did end up touching the transform, repl and babel-jest projects, along with various e2e tests, so you might want to have a look to see if you agree with the changes done to the tests.

I think the only thing that might validate a change is my question on the wording of what is required to implement, as I was a bit unsure myself what you really meant here.

(*) "handles functions that close over outside variables" times out on the Windows builds and "notify › does not report --notify flag" fails on macOS due to using too much time, otherwise it should look pretty good. Pretty sure none of these are related to my changes (?).

[SimenB]

Trang fødsel, indeed 😀

Both of those tests are somewhat flaky (I just pushed some retrying to main in the hopes to stabilize at least one of them).

[SimenB]

why is this needed?

[fatso83]

Well, it was either that or const babelJestTransformer = babelJest.createTransformer(); and babelJestTransformer.getCacheKey(...) (or something similar).

Since we have no other exports than createTransformer, you would otherwise need to write babelJest.createTransformer().getCacheKey(...) in every location that used babelJest.getCacheKey(...) previously.

Not sure what you prefer. Since the transformer was not used for anything besides calling its getCacheKey() method, I thought removing it and just keeping the method through destructuring was fine.

[SimenB]

why not babelJest.getCacheKey ? It should be on the default export, no?

[fatso83]

Since removing all but the factory method was part of this change.

I can of course revert it, but then those exports would just be for the sake of the tests (AFAIK?); the jest-transform package will not touch any of those exports when it sees createTransformer and neither will the CLI, so I thought keeping test-only exports did not make sense from that perspective. I might be wrong, of course 😄

[fatso83]

Hope to have addressed all non-code related feedback. Mostly used your suggested prose, apart from introducing some full stops and minor adjustments to make the sentences shorter and easier to read.

With regards to the code changes you wondered about, which mostly stems from the fact that exports that are not used outside of tests have been removed, I am not sure if you think I should revert any of those or if it made sense.

[SimenB]

Thanks! What I'm alluding to here is that babelJest.getCacheKey should work fine, there's not need to call createTransformer first (I think!)

[fatso83]

And I am saying that functionality was removed in this PR 😄 Sorry if being unclear! See https://github.com/facebook/jest/pull/12407/files#diff-5f463bc5dbe428998302335614e9bbed3b4eb3310a6075767e1b432c161d9e71L273

It used to be:

const transformer: SyncTransformer<TransformOptions> = {
  ...createTransformer(),
  createTransformer
}

export default transformer;

Note the ...createTransformer(). That is what ensures that babelJest.getCacheKey exists.

All of the fields from ...createTransformer() would be ignored by Jest itself when it sees createTransformer. Since they are not used in any actual non-test code, I thought it did not make sense to expose them, since you could always just instantiate a new transformer in the tests. Once their use is gone from tests, no code would use them. So that's the reasoning behind removing them. I could keep them, of course, which would keep the tests unchanged, but what point is there in testing exports that will never be used?

[fatso83]

Thanks a bunch for thorough feedback.

[fatso83]

Well, it was either that or const babelJestTransformer = babelJest.createTransformer(); and babelJestTransformer.getCacheKey(...) (or something similar).

Since we have no other exports than createTransformer, you would otherwise need to write babelJest.createTransformer().getCacheKey(...) in every location that used babelJest.getCacheKey(...) previously.

Not sure what you prefer. Since the transformer was not used for anything besides calling its getCacheKey() method, I thought removing it and just keeping the method through destructuring was fine.

[SimenB]

Sorry, missed the last push!

[SimenB]

thanks! veeeeeery close 😀

My last general question is if we should use admonitions int the docs? I feel like they might help break up the text a bit and make it more digestible? might be wrong, tho!

[fatso83]

My last general question is if we should use admonitions int the docs?

I am change blind 🙈 Can someone do that in a separate PR? I think I am blind to what is better, more clear, etc at this point. And especially whether or not to add some color blocks and where to put them 🤷

[SimenB]

Yeah, no problem 👍

[fatso83]

Hmm ... this is stale, but I am not sure why 🤷 Should I address something or are we good ⬆️?

[SimenB]

Should be good - I've been away on vacation, so haven't had time to properly review

[fatso83]

Totally understandable; I was just not sure if this was blocking due to me or not, since it seemed everything substantial had been addressed.

[SimenB]

thanks!

[SimenB]

https://github.com/facebook/jest/releases/tag/v28.0.0-alpha.8

[github-actions]

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.