[Bug]: Code for inline-snapshots incurs performance penalty even when the feature is not used

swcm-mnestler commented 1 year ago

Version

29.4.1

Steps to reproduce

Clone reproducer at https://github.com/swcm-mnestler/jest-inline-snapshots-reproducer and install modules with npm ci
Measure time to require original, unpatched version of jest-snapshot with npm run unpatched
Compare with patched version with npm run patched
The time difference should be roughly 100ms

Expected behavior

I expect tests which do not use (inline-)snapshots not to have to wait ~100ms for InlineSnapshots.js to finish importing.

Actual behavior

Every run of jest is slowed by ~100ms because InlineSnapshots.js is loaded eagerly in multiple locations. For example, the first time jest-snapshot (and therefore InlineSnapshots.js) is imported in my runs is to fetch the EXTENSION (.snap).

Additional context

If you have a single-project setup, this will not have a significant impact, but we use a monorepo with ~100 subprojects. When we start a clean test run, this means the ~100ms translates to 10s overhead for a feature we don't use.

The reason this file in particular takes so long to import is because of the various babel dependencies it pulls in. I had a look at some of the other typescript-transpiled files, and it seems that some of the require calls are wrapped in a lazy construct- e.g.:

function _crypto() {
  const data = require('crypto');
  _crypto = function () {
    return data;
  };
  return data;
}

Maybe this is the way to go here as well?

Environment

System:
    OS: Linux 5.10 Ubuntu 22.04.1 LTS 22.04.1 LTS (Jammy Jellyfish)
    CPU: (8) x64 11th Gen Intel(R) Core(TM) i7-1185G7 @ 3.00GHz
  Binaries:
    Node: 16.18.1 - (fnm)/bin/node
    Yarn: 1.22.19 - (fnm)/bin/yarn
    npm: 8.19.2 - (fnm)/bin/npm

github-actions[bot] commented 1 year ago

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

SimenB commented 1 year ago

jest-runtime should only be imported once per worker, so the hit of importing jest-snapshot shouldn't scale linearly with number of tests (unless you launch jest multiple times). But there might be other things we can do as well

swcm-mnestler commented 1 year ago

unless you launch jest multiple times

Yep, that's exactly what we're doing in our nx monorepo. I think this specific scenario wouldn't be too common to warrant any effort, but it also affects single-test runs during development.

michael-nestler commented 1 year ago

Found out two aspects of this:

Usually, imports in Jest are rewritten to be evaluated lazily - except for packages in INLINE_REQUIRE_EXCLUDE_LIST
Even if lazy imports were enabled for jest-snapshot, this does not apply to the babel imports because they use requireOutside.

Maybe the babel-plugin-jest-require-outside-vm can be modified to support lazy imports? From a glance at the original babel implementation, it seems to rewrite the entire file to replace any calls to imported values with the lazy-loading function call. A simpler possibility would be to manually perform lazy imports for the babel modules.

jestjs / jest