jest icon indicating copy to clipboard operation
jest copied to clipboard

feat: bundle Jest's modules with webpack

Open SimenB opened this issue 3 years ago • 3 comments

Summary

Bundling our modules allows us to improve our startup time by avoiding a bunch of extra I/O. Optimized code also should run faster (in theory), but might be worth it to drop that for readable code in node_modules?

Anyways, there are (at least) 2 outstanding issues.

  1. jest-worker needs to pass the path to a file to child_process so it can be executed. I've been unable to make Webpack split out a separate file on disk, then return an absolute path to it. Might need to do very custom stuff just for jest-worker, not sure (if possible, keep just plain babel run for this module for now, if everything else works. Hard to tell since without jest-worker, we cannot run anything at all)
  2. There are some places we reach into build on purpose (such as for jest-circus/runner). We need to do something Clever :tm: here. (I haven't bothered thinking too much about as I've swapped between fighting rollup and webpack the last few days).

Any and all help greatly appreciated!

Problem areas: https://github.com/facebook/jest/blob/29534331e2d3d3f70cf153d7e53569d9dd80ecfe/packages/jest-worker/src/workers/NodeThreadsWorker.ts#L61 & https://github.com/facebook/jest/blob/29534331e2d3d3f70cf153d7e53569d9dd80ecfe/packages/jest-worker/src/workers/ChildProcessWorker.ts#L86

Note that worker_threads can receive source code as string, so I wondered if just inlining could work (albeit still a separate bundle), but child_process does not, so doesn't help Another alternative is to inline as bundled string, then write to disk during load. Feels wrong tho


Speaking of rollup, I cannot make it understand require, and it also barfs on import thing = require('thing') even though babel is supposed to compile it away. Anyways, let's try webpack first.

Test plan

Green CI (at some point)

SimenB avatar Feb 09 '22 23:02 SimenB

Speaking of rollup, I cannot make it understand require, and it also barfs on import thing = require('thing') even though babel is supposed to compile it away

You might need to enable the transformMixedEsModules option from @rollup/plugin-commonjs https://github.com/rollup/plugins/tree/9874740ca685b1600e4319365c21e8c1131e9d95/packages/commonjs#transformmixedesmodules

merceyz avatar Feb 11 '22 12:02 merceyz

(just rebased after #12636, I'm not currently planning to work on this)

SimenB avatar Apr 27 '22 12:04 SimenB

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days.

github-actions[bot] avatar Jan 21 '23 23:01 github-actions[bot]

Green! 🎉

SimenB avatar Feb 13 '23 17:02 SimenB

On my machine this shaves off about 10% when running yarn jest packages/expect in this repo.

main:

  Time (mean ± σ):      3.457 s ±  0.351 s    [User: 4.286 s, System: 0.496 s]
  Range (min … max):    3.249 s …  4.429 s    10 runs

This PR:

  Time (mean ± σ):      3.151 s ±  0.060 s    [User: 3.684 s, System: 0.480 s]
  Range (min … max):    3.073 s …  3.249 s    10 runs

Turning on minification seems to improve the results slightly.

  Time (mean ± σ):      3.130 s ±  0.077 s    [User: 3.650 s, System: 0.495 s]
  Range (min … max):    3.019 s …  3.248 s    10 runs

Hard to tell if that's any real improvement or not, tho. And having minification caused quite a bit of tests to fail (I suspect it's due to munging, but not 100% sure) so I'd like to explore that in a separate PR anyways.

SimenB avatar Feb 13 '23 17:02 SimenB

Since we use exports I'm thinking this is not a breaking change. If people wanna use something internal that's not exposed we can revisit (and either revert until Jest 30 or expose whatever it is people are using).

SimenB avatar Feb 13 '23 18:02 SimenB

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
Latest commit 06038a24eff9631ac07186fbd2b934166dc3158c
Latest deploy log https://app.netlify.com/sites/jestjs/deploys/6509fe5fa3a7050008dcd7a5
Deploy Preview https://deploy-preview-12348--jestjs.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Sep 19 '23 12:09 netlify[bot]

A full test run on my machine goes from ~88 seconds to ~73 seconds (without transform cache). Promising 🙂

SimenB avatar Sep 19 '23 13:09 SimenB

A full test run on my machine goes from ~88 seconds to ~73 seconds (without transform cache). Promising 🙂

I was investigating another issue and found that it seems debugging is not working, we are not generating source maps for bundled. I'm not sure if the increased speed has anything to do with it. 🤦‍♂️

To be honest, I'm not sure what I should do. In theory it would be more convenient to not bundle when developing locally, but this would require keeping both build processes and having the tests accept their line numbers. Do you have any suggestions?

liuxingbaoyu avatar Sep 25 '23 09:09 liuxingbaoyu

Happy to produce sourcemaps, but it breaks some tests with filenames in the snapshots. Probably worth it?

SimenB avatar Sep 25 '23 09:09 SimenB

If we also bundle during development, then just emitting the source map all the time should not break tests.

liuxingbaoyu avatar Sep 25 '23 09:09 liuxingbaoyu

But there is still a problem in this case. Maybe we should not let users load the source map by default, which may cause us to test two situations at the same time.

liuxingbaoyu avatar Sep 25 '23 09:09 liuxingbaoyu

Yep.

Debugging is broken due to using workers, I think. https://github.com/jestjs/jest/pull/14085#issuecomment-1689988616

SimenB avatar Sep 25 '23 09:09 SimenB

I'm not sure if what I'm experiencing is related to this, I've been debugging under multi-threading before this. 🤦‍♂️ While I can put breakpoints in the tests, it doesn't work in the jest source code unless I put the debugger manually. image

liuxingbaoyu avatar Sep 25 '23 09:09 liuxingbaoyu

I just worked with the debugger now while putting together https://github.com/jestjs/jest/pull/14578 - breakpoints in jest sources worked fine. I use --inspect-brk and hit Continue once the debugger connects, maybe that's the difference? No manual debugger statements

SimenB avatar Sep 27 '23 07:09 SimenB

image image Unfortunately I still can't get a break point in the ts file.

liuxingbaoyu avatar Sep 27 '23 08:09 liuxingbaoyu

Ah, within the ts file! That probably needs source maps. I always debug in the chrome dev tools, never within the IDE. So haven't noticed 😅

SimenB avatar Sep 27 '23 10:09 SimenB

Yeah. VSCode's debugger can automatically attach to child processes. :) image image

I'll try to get the tests to accept the source map, it shouldn't be too complicated,

liuxingbaoyu avatar Sep 28 '23 01:09 liuxingbaoyu

Awesome!

We have some "absolute path to relative path" stuff for stacktraces, and stripping internal frames. That's where I'd assume any issues would arise - beyond that it should be fine 🙂

SimenB avatar Sep 28 '23 09:09 SimenB

One regression: https://github.com/nodejs/cjs-module-lexer/issues/88

In practice, this breaks jest-watch-typeahead.

Unfortunate that TS types don't have a default export of all the things - not sure how to reconcile named exports in type exports with broken named exports in native ESM...

SimenB avatar Oct 09 '23 08:10 SimenB

Fix for the above was to have our own ESM exports, bypassing the lexer entirely: #14661

Released in https://github.com/jestjs/jest/releases/tag/v30.0.0-alpha.1

SimenB avatar Oct 30 '23 13:10 SimenB

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[bot] avatar Nov 30 '23 00:11 github-actions[bot]