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: bundle Jest's modules with webpack #12348

Merged
merged 10 commits into from
Sep 19, 2023
Merged

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Feb 9, 2022

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 ™️ 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 SimenB marked this pull request as draft February 9, 2022 23:01
}

// inspired by https://framagit.org/Glandos/webpack-ignore-dynamic-require
class IgnoreDynamicRequire {
Copy link
Member Author

Choose a reason for hiding this comment

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

getting webpack to to leave require.resolve alone (as far as I can tell it never behaves like node's builtin one, which is the behavior I want for everything not a local require) is painful and this is spotty. Seems to work fine, tho

@merceyz
Copy link
Contributor

merceyz commented Feb 11, 2022

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

@SimenB
Copy link
Member Author

SimenB commented Apr 27, 2022

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

@SimenB SimenB added the Pinned label Apr 27, 2022
@github-actions
Copy link

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.

@SimenB
Copy link
Member Author

SimenB commented Feb 13, 2023

Green! 🎉

@SimenB SimenB marked this pull request as ready for review February 13, 2023 17:36
@SimenB
Copy link
Member Author

SimenB commented Feb 13, 2023

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
Copy link
Member Author

SimenB commented Feb 13, 2023

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).

@netlify
Copy link

netlify bot commented Sep 19, 2023

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 06038a2
🔍 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.

@SimenB SimenB enabled auto-merge (squash) September 19, 2023 20:06
@SimenB SimenB merged commit 70340d0 into jestjs:main Sep 19, 2023
59 of 60 checks passed
@SimenB SimenB deleted the webpack-bundle-jest branch September 19, 2023 22:20
@liuxingbaoyu
Copy link
Contributor

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?

@SimenB
Copy link
Member Author

SimenB commented Sep 25, 2023

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

@liuxingbaoyu
Copy link
Contributor

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

@liuxingbaoyu
Copy link
Contributor

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.

@SimenB
Copy link
Member Author

SimenB commented Sep 25, 2023

Yep.

Debugging is broken due to using workers, I think. #14085 (comment)

@liuxingbaoyu
Copy link
Contributor

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

@SimenB
Copy link
Member Author

SimenB commented Sep 27, 2023

I just worked with the debugger now while putting together #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

@liuxingbaoyu
Copy link
Contributor

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

@SimenB
Copy link
Member Author

SimenB commented Sep 27, 2023

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 😅

@liuxingbaoyu
Copy link
Contributor

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,

@SimenB
Copy link
Member Author

SimenB commented Sep 28, 2023

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
Copy link
Member Author

SimenB commented Oct 9, 2023

One regression: nodejs/cjs-module-lexer#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
Copy link
Member Author

SimenB commented Oct 30, 2023

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

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 Nov 30, 2023
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.

4 participants