Initial support for Resource Queries

[peter-leonov]

UPDATE: See this comment for the changes plan and the latest progress.

TL;DR; this PR needs an architecture decision for further development 🙏 DONE in the comment linked above 🎉

Backstory

Resource Queries is a relatively recent trend defined by support by WebPack's import() and in ES6 import(). Having Resource Queries supported in dynamic import() means it should be also supported in static import * from 'module'. It is expected that developers will build features on top of the web driven semantics of import, so it would be of use to have basic support for Resource Query aware plugins in Jest as well.

As decided in #4549 this PR should only demonstrate how minor would be the impact fo the Solution 3 described below. A scope of the final goal is defined in #4181 (comment).

Description

Firstly, here go file path use cases in the core test running process:

• read / fstat the file;
• use as a part of a cache key;
• getting file extension for transformation;
• matching agains RegExp to ignore.

Solution 1: Smart filename

Full scale solution satisfying all use cases would be to teach a user defined resolver() to return an object {path, id} instead of a string, and then refactor all the code which uses type Path, as currently the full module path is (expectedly) used as a uniq file identifier across many modules;

Good example is the getScriptCacheKey() at script_transformer.js. It needs a module file path to get it's mtime and also the file id to add it to the key, as for the same physical file the modification time will also be the same leading to the same cache keys for two different resources queries. Here is how it might look like:

const getScriptCacheKey = (file, config, instrument: boolean) => {
  const mtime = fs.statSync(file.path).mtime;
  return file.id + '_' + mtime.getTime() + (instrument ? '_instrumented' : '');
};

Solution 2: Smart resolver

Less aggressive approach might be to just teach Jest to transform original file path before use in functions from fs and path modules with new method of resolver (say, getFsPath()). This could enable a native support for Resource Queries by default or any other file path overloading. This will require changing code in few places where Jest runtime touches filesystem or needs a real file name by injecting that getFsPath().

The example with getScriptCacheKey() would then look like this:

const getScriptCacheKey = (filename, config, instrument: boolean) => {
  const mtime = fs.statSync(config.resolver.getFsPath(filename)).mtime;
  return filename + '_' + mtime.getTime() + (instrument ? '_instrumented' : '');
};

Solution 3: Too smart fs module

The most general and least intrusive solution (but also kind of hacky) is to make the fs module configurable, so that a plugin developer can provide a version of fs which tolerates the resource query appendix. This might be a first baby step towards a full Resource Query support. This solution breaks though two use cases. The one where a module file name extension is used to select a transformer (path.extname('a.out?query') return '.out?query'). The other one is matching a module file path against an ignore RegExp (as /test/.'/path/to/load.js?ab_test=1'. returns true). This could be partially fixed by prefixing a file path with filters (like webpack does) internally instead of appending.

If you know how to better resolve this architectural challenges, please, help 😊

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[peter-leonov]

Anyone? :) Having support for Resource Queries will make Jest even more modular. Imagine, that users could pass parameters to resolvers, runtimes or transformers. This allows a bunch of applications, esp. for dynamic instrumentation.

[SimenB]

@rubennorte @rafeca any opinions here? Would something like this be useful for e.g. metro?

[codecov-io]

Codecov Report

Merging #6282 (0e03d34) into master (2a4b073) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6282      +/-   ##
==========================================
+ Coverage   64.92%   64.98%   +0.05%     
==========================================
  Files         288      283       -5     
  Lines       12199    12108      -91     
  Branches     3024     2992      -32     
==========================================
- Hits         7920     7868      -52     
+ Misses       3639     3604      -35     
+ Partials      640      636       -4     

Impacted Files	Coverage Δ
packages/jest-runtime/src/index.ts	64.50% <100.00%> (-0.78%)	⬇️
packages/jest-transform/src/ScriptTransformer.ts	69.64% <100.00%> (ø)
packages/jest-haste-map/src/crawlers/node.ts	82.89% <0.00%> (-3.78%)	⬇️
packages/jest-reporters/src/coverage_reporter.ts	51.21% <0.00%> (-2.68%)	⬇️
packages/jest-snapshot/src/inline_snapshots.ts	80.39% <0.00%> (-1.61%)	⬇️
...es/pretty-format/src/plugins/ReactTestComponent.ts	81.81% <0.00%> (-1.52%)	⬇️
packages/expect/src/jestMatchersObject.ts	75.00% <0.00%> (-0.87%)	⬇️
packages/jest-console/src/CustomConsole.ts	77.08% <0.00%> (-0.47%)	⬇️
... and 68 more

---

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 2a4b073...23c174f. Read the comment docs.

[peter-leonov]

I see some movement (#7349, #7350, #7313) to make the interface to the file system more abstract. Maybe it is now a good time to add support for Resource Queries by abstracting the file paths too? 😅

[peter-leonov]

Anyone 🙏😤😇

[veitbjarsch]

This feature would be awesome. I hate just mocking every ressource query.

[bitttttten]

How are you mocking resource queries @veitbjarsch ? For me Jest will complain that the module cannot be found.

Cannot find module '../assets/user_background.jpg?min=320&max=800&steps=3' from 'index.test.tsx'

[trymoto]

Sadly, no motion around this issue

@peter-leonov are you still driving this?

[peter-leonov]

I’d love to drive, but I’m not sure what else to do and who to poke 😅

[trymoto]

I think we can start by reviewing some conflicts this has with master now

[peter-leonov]

Being there, done that:

peter-leonov force pushed changes to this branch 8 months ago

[SimenB]

So sorry about the slow response here @peter-leonov!

We'll need this for ESM support (#9430), see https://nodejs.org/api/esm.html#esm_url_based_paths. I'll try to find the time to dive into this at some point, but if you have time to work on it, I'd love to help land changes!

---

I don't really care what bundlers do with queries (the resolver is pluggable, so people can swap it out if they want something for webpack specific syntax), we should follow whatever the ESM spec tells us to do. From node's short docs above it's just returning a separate module instead of the cached one. Does the fuller ESM spec say anything more? Should it be added to import.meta or something?

Of the 3 approaches outlined in to OP I think we should go for option 2 - it's literally a resolution problem.

Thinking about it, I disagree with myself. jest-runtime has the responsibility to resolve, load, evaluate, execute and cache modules, so I think the complexity can be confined there. It should just strip out any query params it encounters, but use it to vary its cache. Other parts of Jest should just be able to ignore it. All require (and future import) calls go through an implementation created in jest-runtime, so confining the changes to that class should be relatively simple without the rest of the system having to care at all. Except for https://github.com/facebook/jest/blob/823677901b9632dbfb9397bf5eef2d32b78527a4/packages/jest-haste-map/src/lib/dependencyExtractor.ts which is jsut a regex over source code - it should ignore queries.

For require calls I think we should just throw - it's not valid

[peter-leonov]

@SimenB, thanks for spending time to dig this one out! I'm still up to finish this one, will look into closer asap.

[peter-leonov]

Rebased the branch (my own migration experience from Flow to TS helped a lot).

Other parts of Jest should just be able to ignore it.

True, but it still is important to keep that original file name with the query suffix somewhere in the API for other parts of Jest (or it's plugings) to inspect, right?. Otherwise we loose the whole point of having queries in the first place: to modify the module's perceived content. For example, I have a use case for my own tool (kinda like rewire plugin) for Jest where a babel plugin does some additional transformations based on the query parameter.

What do you think, @SimenB?

[SimenB]

We can provide it to custom resolvers maybe? That way you can plug in some other resolver if you want, and use the query to do something. It might make sense to also provide it to code transforms, that way you can use it to transform files in any way you want (like how file-loader uses queries).

Default behaviour in Jest should match Node.js - so it should be to not find the module when using CJS and to use it as a cache vary for ESM. Since we don't support ESM yet, for now it should cause a MODULE_NOT_FOUND.

Some interesting discussion for how it'll affect ESM here: nodejs/modules#417

[peter-leonov]

We can provide it to custom resolvers maybe?

That's a good idea and it will require a quite significant change in the Resolver API. Currently, if I'm not mistaken, the chain of module resolution on higher level looks like: a test calls require which calls smart code in jest-runtime, which calls jest-resolve which calls in the end the defaultResolver() or any custom resolver.

The issue why a more significant change is required is that the tail of the chain described above has type (Config.Path, ResolverOptions) => Config.Path where Config.Path is just a string which is used everywhere as is. The smart caching implementation in jest-runtime and some other modules (I believe the structure did not change dramatically over the last two years) would require a additional information to the plain string as those modules in many cases give the resolved path directly to the node fs module.

I believe, that making such an API change in now clearly required to support ESM modules, but I'd like to first agree on a hight level solution first and then only continue. As I'm very new to the Jest dev process, maybe you can help me here with a bigger picture view?

[peter-leonov]

Cool, thanks for the instant replies! Starting getting my hands dirty then.

The plan is as simple as:

• Remove the POC implementation of the initial PR;
• Introduce the new type Config.ResolvedPath = {…}
• Use Config.ResolvedPath everywhere the resolved path is required starting from a good seed point (getScriptCacheKey is a good candidate);
• Provide the real values
• Replace all the calls to realpath() (see in _getRealPath()) with the only once resolved property in the Config.ResolvedPath struct;
• Update the type to match what we've agreed on;
• Do not expose yet the query to the external resolvers (needs clarification)
• Make sure the caching and the re-running of modules work as expected;
• Update jest-pnp-resolver to use Config types instead of hardcoded string;

And maybe:

• Try giving the needed info to transform and friends.

Next steps:

• think of backwards compatible changes to things like shouldTransform() and shouldInstrument() which expect a file extension as the last part of a file path.

[peter-leonov]

Hey @SimenB, I'd like to check with you if this PR is going in the right direction.

[SimenB]

I've let this sit in the slow cooker for some time now, and I now think we don't need to change the API of the resolver here.

I think we can just

1. pop of any query
2. resolve as normal
3. re-add the query when passing the resolved path back.

so you'll do Resolver.findNodeModule('./bla.js?hey=there') and get back /some/absolute/path/bla.js?hey=there. Then it's up to the caller what to do about the query

I think it makes sense for jest-runtime to extract the query so it can be used to vary the cache and pass it as its own argument to @jest/transformer so it can pass it on to transformers.

Thoughts?

[SimenB]

prettier/prettier#968 😞

[SimenB]

There's also https://github.com/tc39/proposal-module-attributes to consider

[github-actions]

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

[SimenB]

@peter-leonov hiya! 👋 is this thing still alive? Minimal support for query was added in #9914. As for import assertions (which the above linked proposal has been renamed to), #12755 has a start on that.

[github-actions]

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

[pcorpet]

Any chance we could get this PR merged at one point please?

[github-actions]

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

[pcorpet]

Any chance we could get this PR merged at one point please?