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(runtime): populate require.cache #9841

Merged
merged 8 commits into from
Apr 20, 2020
Merged

Conversation

antongolub
Copy link
Contributor

@antongolub antongolub commented Apr 19, 2020

Summary

This PR brings more expected behavior for require.cache: populates with loaded modules data, but protects from modification.

In my specific practical case I need to verify the huge app context initialization (app runner, low level api injections, hooks, DI IoC container, and so on). Checking all side effects requires tons of code, but in some cases this annoying routine can be replaced by a simple module loading check.

Relates: #6725, #6066, #5741, #5120.

Test plan

@codecov-io
Copy link

codecov-io commented Apr 19, 2020

Codecov Report

Merging #9841 into master will increase coverage by 0.00%.
The diff coverage is 85.71%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9841   +/-   ##
=======================================
  Coverage   64.46%   64.47%           
=======================================
  Files         289      289           
  Lines       12327    12333    +6     
  Branches     3049     3053    +4     
=======================================
+ Hits         7947     7952    +5     
- Misses       3740     3741    +1     
  Partials      640      640           
Impacted Files Coverage Δ
packages/jest-runtime/src/index.ts 60.61% <85.71%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1039ed1...6b595f5. Read the comment docs.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Oh, very cool! I never thought we'd actually add support for this property, but this is actually pretty nice. Lays a path for respecting deletions as well at some point 👍

Mind updating the changelog as well (not that I'll be making a release in a few minutes, so you'll get a conflict in the changelog)

packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks!

@antongolub
Copy link
Contributor Author

@SimenB, hold on. I'm going to refactor some piece. There's no need for separate cache object when proxy is used.

@SimenB
Copy link
Member

SimenB commented Apr 20, 2020

Ah ok, cool! 👍

packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
packages/jest-runtime/src/index.ts Outdated Show resolved Hide resolved
@SimenB
Copy link
Member

SimenB commented Apr 20, 2020

@antongolub I think this looks great, ready to go?

@antongolub
Copy link
Contributor Author

@SimenB, Iet's merge.

@SimenB SimenB merged commit 470ef2d into jestjs:master Apr 20, 2020
@SimenB
Copy link
Member

SimenB commented Apr 20, 2020

Thanks!

@SimenB
Copy link
Member

SimenB commented Apr 21, 2020

@antongolub hmm, looking at CI I'm seeing this:

image

Ideas? 😀

Wait, might only be in #9853, I've probably messed it up somehow

@SimenB
Copy link
Member

SimenB commented Apr 21, 2020

No, happens on master as well. Happens in a few files, but the worst one is, by far, packages/jest-core/src/__tests__/watch-file-changes.test.ts. Would you be able to take a look?

iliapolo added a commit to aws/aws-cdk that referenced this pull request Apr 29, 2020
Couple of things here:

**require.cache**

In version `v25.5.0`, jest introduced an [implementation](jestjs/jest#9841) of their own to the `require.cache` object. It seems that it doesn't handle caching `json` modules properly, which causes our code to fail because `mod.paths` is `undefined` when we are querying for `json` files that were required (for example `package.json` or `cloud-assembly.schema.json`).

https://github.com/aws/aws-cdk/blob/07fe642e38118e24837b492fa182737fc41bb429/packages/%40aws-cdk/core/lib/private/runtime-info.ts#L66  

```console
TypeError: Cannot read property 'map' of undefined
```

The "fix" was to add a null check to prevent failure when looking up modules who don't have the `paths` property defined. 

Note that this was only observed in test environments using `jest > v25.5.0`, not during actual runtime of `cdk synth`. This is because `jest` manipulates the built-in `require` module of `nodejs`.

**graceful-fs**

In version `v25.5.0`, jest [added](jestjs/jest#9443) a dependency on the `graceful-fs` package. The version they depend on (`4.2.4`) differs from the version that we bring (`4.2.3`). This caused the `graceful-fs` dependency that comes from `jest` no to be deduped by node at installation. In turn, this caused multiple copies of the library to be installed on disk. 

The `graceful-fs` package monkey patches the `process.chdir` and `process.cwd` functions with a cached version for better performance.

For reasons not completely clear to us, the existence of multiple copies of the module, caused `process.cwd()` to return incorrect cached results, essentially always returning the directory that the process was started from, without consideration to calls to `process.chdir`. This broke `decdk` (and possibly many more) tests which rely on `process.chdir`.  

Fixes #7657
@adamwathan
Copy link

In Tailwind CSS we rely on being able to delete keys in require.cache to invalidate stale modules when used in a watching process, like with webpack:

https://github.com/tailwindcss/tailwindcss/blob/master/src/index.js#L54

This change is causing us to see warnings in our test runs now.

Nothing is actually broken but it would be great to get rid of the warnings — any suggestions on the best way to do it?

iliapolo added a commit to aws/aws-cdk that referenced this pull request Apr 29, 2020
Couple of things here:

**require.cache**

In version `v25.5.0`, jest introduced an [implementation](jestjs/jest#9841) of their own to the `require.cache` object. It seems that it doesn't handle caching `json` modules properly, which causes our code to fail because `mod.paths` is `undefined` when we are querying for `json` files that were required (for example `package.json` or `cloud-assembly.schema.json`).

https://github.com/aws/aws-cdk/blob/07fe642e38118e24837b492fa182737fc41bb429/packages/%40aws-cdk/core/lib/private/runtime-info.ts#L66

```console
TypeError: Cannot read property 'map' of undefined
```

The "fix" was to add a null check to prevent failure when looking up modules who don't have the `paths` property defined.

Note that this was only observed in test environments using `jest > v25.5.0`, not during actual runtime of `cdk synth`. This is because `jest` manipulates the built-in `require` module of `nodejs`.

**graceful-fs**

In version `v25.5.0`, jest [added](jestjs/jest#9443) a dependency on the `graceful-fs` package. The version they depend on (`4.2.4`) differs from the version that we bring (`4.2.3`). This caused the `graceful-fs` dependency that comes from `jest` no to be deduped by node at installation. In turn, this caused multiple copies of the library to be installed on disk.

The `graceful-fs` package monkey patches the `process.chdir` and `process.cwd` functions with a cached version for better performance.

For reasons not completely clear to us, the existence of multiple copies of the module, caused `process.cwd()` to return incorrect cached results, essentially always returning the directory that the process was started from, without consideration to calls to `process.chdir`. This broke `decdk` (and possibly many more) tests which rely on `process.chdir`.

Fixes #7657
@SimenB
Copy link
Member

SimenB commented Apr 30, 2020

No really good way of suppressing the warning. I'm currently leaning towards silencing the warning, and maybe add it back for Jest 26. The code you link to can be supported though (deleting single entries from the cache), we just haven't done so.

We can discuss this is #9916 though

@github-actions
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 May 11, 2021
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.

5 participants