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

jest-haste-map: deprecate functional ignorePattern and use it in cache key #4063

Merged
merged 2 commits into from
Jul 18, 2017

Conversation

jeanlauliac
Copy link
Contributor

I plan to revert ignorePattern to only being a strict RegExp, after removing the function use case from React Native and Metro bundler. The reason for getting rid of the function use case is that ignorePattern should be part of the cache key, and a function is not analysable at runtime. If it can only be a RegExp, then we can use ignorePattern.toString() as part of the cache key.

The reason it needs to be part of the cache key is because the cache needs to be discarded—or at least reevaluated—when the ignore pattern changes. Otherwise, we can be missing some new modules that should be included, or the reverse, we can be including modules that should now be ignored. I have observed considerable trouble caused by this issue. For example, in React Native, people would reload a project and it wouldn't find a module, or, duplicates modules would be detected while in fact one of them should have been ignored.

This changeset add a deprecation notice for the functional use case (so that we can release this as a minor/revision), and starts using the string representation of the ignore pattern in the cache key (so that we can get a correct behavior as soon as possible for callsites that do already use a RegExp).

See also #2957, that introduced ignorePattern as a function.

Alternatively, we could require callsites to provide their own cache key if they do want to use a function, but this makes it more complicated and I'm not sure there's really any other callsites than React Native that does that, that we will fix ourselves as well.

@cpojer
Copy link
Member

cpojer commented Jul 18, 2017

Please run prettier before merging.

@jeanlauliac jeanlauliac merged commit bf58182 into jestjs:master Jul 18, 2017
@codecov-io
Copy link

Codecov Report

Merging #4063 into master will decrease coverage by <.01%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4063      +/-   ##
==========================================
- Coverage   60.42%   60.41%   -0.01%     
==========================================
  Files         196      196              
  Lines        6764     6766       +2     
  Branches        6        6              
==========================================
+ Hits         4087     4088       +1     
- Misses       2674     2675       +1     
  Partials        3        3
Impacted Files Coverage Δ
packages/jest-haste-map/src/index.js 92.63% <50%> (-0.34%) ⬇️

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 7b9b5b3...085b11a. Read the comment docs.

@SimenB
Copy link
Member

SimenB commented Jul 18, 2017

a function is not analysable at runtime

You can call toString on it to get the implementation. I'm sure there's no reason to keep it either way, just thought to point it out

@jeanlauliac
Copy link
Contributor Author

jeanlauliac commented Jul 19, 2017

You can call toString on it to get the implementation. I'm sure there's no reason to keep it either way, just thought to point it out

toString on a function is not exhaustive: for example in React Native the function typically doesn't change, but the function itself is using a RegExp from its scope, that can change. Using toString on the function doesn't capture the changes happening in that RegExp or any other variable of the function's scope.

@SimenB
Copy link
Member

SimenB commented Jul 19, 2017

Non-pure functions suck :(

@jeanlauliac
Copy link
Contributor Author

Well afaik. it is not related to function purity in that case, because the RegExp the function depend on is actually immutable. It never changes at runtime. But, it could change from a run to another of the whole bundler process, when the config is manually changed in-between :) This happens more often than we think, for example after doing a git pull, and someone changed the configuration. Cache keys are a hard thing to get right.

@SimenB
Copy link
Member

SimenB commented Jul 19, 2017

(this is getting academic 😄).

A pure function is one which returns the same output for the same input. Having some global state (such as using Date.now or closing over a variable from an outer scope) affect the output makes it non-pure. It's not strictly about mutability.

But I agree that just not supporting functions are way easier!

tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017
…e key (jestjs#4063)

I plan to revert `ignorePattern` to only being a strict `RegExp`, after removing the function use case from React Native and Metro bundler. The reason for getting rid of the function use case is that `ignorePattern` should be part of the cache key, and a function is not analysable at runtime. If it can only be a `RegExp`, then we can use `ignorePattern.toString()` as part of the cache key.

The reason it needs to be part of the cache key is because the cache needs to be discarded—or at least reevaluated—when the ignore pattern changes. Otherwise, we can be missing some new modules that should be included, or the reverse, we can be including modules that should now be ignored. I have observed considerable trouble caused by this issue. For example, in React Native, people would reload a project and it wouldn't find a module, or, duplicates modules would be detected while in fact one of them should have been ignored.

This changeset add a deprecation notice for the functional use case (so that we can release this as a minor/revision), and starts using the string representation of the ignore pattern in the cache key (so that we can get a correct behavior as soon as possible for callsites that do already use a `RegExp`).

See also jestjs#2957, that introduced `ignorePattern` as a function.

Alternatively, we could require callsites to provide their own cache key if they do want to use a function, but this makes it more complicated and I'm not sure there's really any other callsites than React Native that does that, that we will fix ourselves as well.
@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 13, 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