Add jest transform for hoisting module mocks #540
Conversation
|
@alangpierce is this something that looks reasonable to you? |
|
Ping :) This is still something that we would value. |
|
I've just tested this branch and it fixes all the problems I had with the current jest plugin and all my tests pass, it would be great for this to be merged! |
|
It's been a while folks! Please merge this one :) |
|
@alangpierce Any chance to release it? |
There was a problem hiding this comment.
Thanks so much for the PR, and sorry for the delay in getting to this! I had been putting it off compared with other Sucrase priorities, and have had a lot of other things going on in my life and have had only occasional bursts of focus time for Sucrase. I've had a few lingering hesitations about this PR (more details below), but after thinking it through more, I think it makes the most sense to merge and release this, since it seems like a safe change and clearly would provide value.
I really appreciate the documentation, tests, thoroughness, and general code quality. I found some small issues that I can fix in a quick follow-up PR.
To explain some of my higher-level hesitations (which ideally I would have surfaced sooner):
The PR breaks line number preservation when the jest transform is enabled.
Sucrase so far has had a guarantee that line numbers are the same before and after transformation, which makes source maps trivial because each line just maps to itself. Because this Jest transform reorders multi-line blocks, it ends up shifting line numbers for most/all of the file, which means that debugger breakpoints won't work correctly.
This is generally a challenging problem with any transform related to reordering, and I remember running into this with the class fields transform as well. I originally implemented it by moving the initializer code to the constructor, but that shifted line numbers. One possible workaround is to join the code snippet to one long line, but that raises some worries around automatic semicolon insertion and needs some complexity around multiline string literals and // comments. My eventual solution for class fields was to wrap each initializer in a method declaration and then call the relevant method.
An analogous approach here may be to transform this code:
import 'moduleName';
jest.mock('a', () => {
console.log('Hi');
});
jest.mock('b');into this:
__init1(); __init2(); import 'moduleName';
function __init1() { jest.mock('a', () => {
console.log('Hi');
}); }
function __init2() { jest.mock('b'); }It's a bit ugly in that it's relying on JS hoisting for function declarations, but I think it should work. It does add some name generation and lookahead complexity to the transform, though, so I think it's fine to release this PR and optionally fix the line number issue in the future.
I don't personally have much experience with Jest.
Sucrase and my employer both use Mocha, so I haven't had as much hands-on experience with Jest and wasn't previously aware of the hoisting issue. I feel a little embarrassed since it's such a widely-used tool, and have been meaning to understand it more deeply. A few months ago I tried switching the Sucrase test suite to use Jest, but the test suite time slowed down from about half a second to about 5 seconds (I believe due to the sandboxing that Jest does), so I decided to stick with Mocha for now. What that means in practice here is that I haven't personally tested this PR in a realistic setting, so I'm relying on the community to ensure quality and report issues.
It's unclear if this transform should be necessary in the first place.
To me, it feels a little strange to write .mock calls below imports and then expect the mocking to happen before the import runs. Babel reorders import statements to the top of the file, so it makes sense that in a Babel setup you'd need an additional transform to reorder the .mock calls even higher. Sucrase, however, follows the TypeScript approach of leaving import statements where they are, so a .mock call before an import statement will preserve that order.
That said, I understand that .mock after imports may be more idiomatic or may play better with editors and other tools. It just feels a little strange to me that this is solved using a code transform.
It's unclear if this transform should be considered in the scope of Sucrase.
Sucrase is generally concerned with non-standard syntax extensions (particularly JSX and TypeScript), though there are certainly exceptions to that. It's probably too big of a scope for Sucrase to support every tool that needs a custom code transform, but it does seem pragmatic to pick the most important tools to support, which was the rationale for the react-hot-loader transform. It's probably fair to count Jest as one of the most important tools to support, but I think generally each tool needs to be decided case-by-case.
To answer a question in the PR summary:
It also doesn't support non-imports transform + jest, but since jest currently doesn't support ESM, that can be added later. Figured the most straight-forward way to do that would be to add global names to the SucraseContext, regardless of which transform is selected. Basically adding an equivalent of importProcessor.getGlobalNames() and getTSImportedNames(tokenProcessor) for when neither imports or typescript processors are active.
I agree that it seems reasonable to skip ESM support for this PR. My main hesitation with adding more information to the SucraseContext by default is the performance cost of computing globals when they aren't needed, so that would be the main thing to be cautious of, but overall the code strategy looks right to me.
| this.resultCode += this.previousWhitespaceAndComments().replace(/[^\r\n]/g, ""); | ||
| this.tokenIndex++; |
There was a problem hiding this comment.
Definitely subtle, but generally makes sense that the appendTokenPrefix/appendTokenSuffix code can cause problems here.
I think we probably want to make the analogous change in removeInitialToken as well, so I'll change that function as a follow-up.
| if (this.importProcessor?.getGlobalNames()?.has(JEST_GLOBAL_NAME)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Just noting that the avoids hoisting if jest is an imported symbol test still passes with this line removed. It looks to me like the import transform gets to the code first so the jest transform never runs on the jest token. But it still probably makes sense to have these lines to be defensive.
Also, according to https://jestjs.io/docs/jest-object, the jest object can be imported, so it's not clear to me that skipping is the best default behavior here. But as an initial pass, I suppose either way is fine, since hopefully it's a rare case anyway. Looks like the babel plugin checks the imported path in that case: https://github.com/facebook/jest/blob/d1ad64c0b5cdf68a73e11a12d7770be7d1381584/packages/babel-plugin-jest-hoist/src/index.ts#L211
| it("transforms jsx in parameters", () => { | ||
| assertResult( | ||
| ` | ||
| import {x, X} from './a'; | ||
| jest.mock('a': number, (arg: string) => ({ | ||
| f(x: string): void { | ||
| return (x: X); | ||
| } | ||
| }): any); | ||
| x() | ||
| `, |
There was a problem hiding this comment.
Looks like you meant to include jsx here but instead have the same flow test case above. I'll fix in a follow-up change.
|
@alangpierce BTW. Is Surcrase support react-refresh? (as you mentioned it support HMR, but I'm unsure react-refresh and HMR are connected) UPD: Oh, I see #590 |
|
@alangpierce and another question. I guess this will be released after you or somebody else fixes the source map issue? As I guess broken debugger is kinda meh and I would rather hoist myself for the time being than using this transform but keep the debugger experience perfect as it was before :) Thanks! :) |
My plan was actually to release this with the broken line numbers, since it only affects people explicitly using the |
This is a follow-up from #540 (review) For the Jest transform, rather than actually moving the `jest.mock` invocations, it's better to transform them in-place and wrap them in functions, then call those functions from the top of the file. That guarantees that line numbers are preserved before and after so that the source map can be correct. I also added the jest transform to the website so that it's easier to manually test. It may be possible to hook up the ts-jest version of the transform to the TypeScript output on the website, but I left that off for now. To test, I made a simple Jest project and mocked a function that returns a number. Without the jest tranform, putting the mock call below the import doesn't work, and with the jest transform it works. Before this change, breakpoints further down in the file (e.g. in the test) didn't work because line numbers were wrong. After this change, breakpoints work. I also manually tested this line to ensure that the chaining transform doesn't break other uses of the `jest` object: ```javascript console.log(jest.spyOn({foo() {}}, 'foo').getMockName()); ```
|
I just put up a line number fix at #608! I did some basic testing, but if anyone has a chance to test it on a real codebase, would be great to hear any feedback or issues. If not, I can do a release soon and any potential issues can be fixed in a follow-up release. |
This is a follow-up from #540 (review) For the Jest transform, rather than actually moving the `jest.mock` invocations, it's better to transform them in-place and wrap them in functions, then call those functions from the top of the file. That guarantees that line numbers are preserved before and after so that the source map can be correct. I also added the jest transform to the website so that it's easier to manually test. It may be possible to hook up the ts-jest version of the transform to the TypeScript output on the website, but I left that off for now. To test, I made a simple Jest project and mocked a function that returns a number. Without the jest tranform, putting the mock call below the import doesn't work, and with the jest transform it works. Before this change, breakpoints further down in the file (e.g. in the test) didn't work because line numbers were wrong. After this change, breakpoints work. I also manually tested this line to ensure that the chaining transform doesn't break other uses of the `jest` object: ```javascript console.log(jest.spyOn({foo() {}}, 'foo').getMockName()); ```
|
Just published as |
|
@alangpierce awesome! Will check tomorrow :) |
Fixes #609 The original implementation of the Jest transform (#540) needed to remove regions of code, but ran into issues with the optional chaining and nullish coalescing transforms, since those transforms would still emit function calls around tokens even if they were removed. The implementation fixed those issues by disabling the optional chaining and nullish coalescing code emit in `removeToken` and `removeInitialToken`. Unfortunately, this broke other cases, like a nullish coalescing call immediately followed by a type token. The nullish coalescing implementation expected `appendTokenSuffix` to be called on the last token even though it was a type token. The changes to `TokenProcessor` actually became unnecessary as of #608 since we no longer are deleting a region of code, so I reverted the two methods back to their original implementation, which fixes the issue.
Fixes #609 The original implementation of the Jest transform (#540) needed to remove regions of code, but ran into issues with the optional chaining and nullish coalescing transforms, since those transforms would still emit function calls around tokens even if they were removed. The implementation fixed those issues by disabling the optional chaining and nullish coalescing code emit in `removeToken` and `removeInitialToken`. Unfortunately, this broke other cases, like a nullish coalescing call immediately followed by a type token. The nullish coalescing implementation expected `appendTokenSuffix` to be called on the last token even though it was a type token. The changes to `TokenProcessor` actually became unnecessary as of #608 since we no longer are deleting a region of code, so I reverted the two methods back to their original implementation, which fixes the issue.
Made an attempt at #539, let me know if this makes sense 😁
It doesn't do all the same checks as babel-plugin-jest-hoist, which verifies that only whitelisted out-of-scope variables are accessed inside module mock factories.
Also had to add
getHoistedCodetoTransformer, to be able to move code up below all the helpers. There might be some nicer way to do this though.It also doesn't support non-imports transform + jest, but since jest currently doesn't support ESM, that can be added later. Figured the most straight-forward way to do that would be to add global names to the
SucraseContext, regardless of which transform is selected. Basically adding an equivalent ofimportProcessor.getGlobalNames()andgetTSImportedNames(tokenProcessor)for when neitherimportsortypescriptprocessors are active.