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

Allow for imports when path includes a query string. Fixes #4181 #4549

Conversation

DylanAndrews
Copy link

@DylanAndrews DylanAndrews commented Sep 27, 2017

Fixes #4181

Summary

Jest fails to apply a transform to a file if the import path includes query parameters. A detailed description can be found here.

Test plan
Added tests to ensure this is functioning properly.

@cpojer
Copy link
Member

cpojer commented Sep 27, 2017

What happens when I want to do: require('foo?a') and require('foo?b')? What will happen in various bundlers that support this syntax? If they are two JS files, will they be the same module instance or will they evaluate the possibly same code twice? Are they to be considered separate modules, or the same ones?

With this change to Jest, what is the behavior and how does it diverge or match those bundlers?

@DylanAndrews DylanAndrews reopened this Sep 27, 2017
@DylanAndrews DylanAndrews force-pushed the feature/transform-file-with-query-parameters branch from 10f255c to c4726a7 Compare September 27, 2017 21:48
@codecov-io
Copy link

codecov-io commented Sep 27, 2017

Codecov Report

Merging #4549 into master will increase coverage by 0.78%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4549      +/-   ##
==========================================
+ Coverage   56.04%   56.82%   +0.78%     
==========================================
  Files         185      186       +1     
  Lines        6268     6305      +37     
  Branches        3        3              
==========================================
+ Hits         3513     3583      +70     
+ Misses       2754     2721      -33     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-resolve/src/index.js 37.27% <100%> (+0.57%) ⬆️
.../eslint-plugin-jest/src/rules/no_disabled_tests.js 94.44% <0%> (-2.78%) ⬇️
packages/jest-diff/src/diff_strings.js 98.86% <0%> (-0.03%) ⬇️
packages/jest-cli/src/test_sequencer.js 100% <0%> (ø) ⬆️
packages/jest-cli/src/jest.js 0% <0%> (ø) ⬆️
...ages/jest-config/src/reporter_validation_errors.js 11.76% <0%> (ø) ⬆️
packages/jest-config/src/utils.js 72.72% <0%> (ø) ⬆️
packages/jest-config/src/deprecated.js 75% <0%> (ø) ⬆️
packages/jest-config/src/resolve_config_path.js 95.45% <0%> (ø) ⬆️
packages/jest-cli/src/test_watcher.js 42.85% <0%> (ø) ⬆️
... and 169 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 5992763...c4726a7. Read the comment docs.

@DylanAndrews
Copy link
Author

DylanAndrews commented Oct 9, 2017

@cpojer At a high-level, my primary goal was to circumvent the error thrown by Jest's resolver for module paths that included query string. For my specific use-case, I do not necessarily care about the resulting imported module. I am mocking the image file entirely.

However, I do believe you raise good questions. Unfortunately, I'm not confident on the best answers to them nor the approach to resolving them. If you have a desired outcome for your scenarios or recommendations on implementing the changes, it'd be appreciated if you provide some guidance. I am happy to explore executing your recommendations.

Thank you in advance. I greatly appreciate your time and the investments your team has made in Jest. Our team loves Jest!

@cpojer
Copy link
Member

cpojer commented Dec 15, 2017

I'm going to close this PR because of the unanswered questions but I am not opposed to merging this if somebody takes the time to fully think this through. For now, I recommend using moduleNameMapper to strip of the bits of the URL at the end.

@peter-leonov
Copy link

@cpojer, do you know if anybody is working on this? I'm ready to start 💪

I've got a nice use case for queries described in #4181 (comment) and also some compatibility analysis in #4181 (comment). Thanks!

@cpojer
Copy link
Member

cpojer commented May 10, 2018

Is this something we can otherwise solve with a custom resolver so that we don't have to ship this within Jest by default?

@peter-leonov
Copy link

peter-leonov commented May 10, 2018

This was my first try too. As far as I understand Jest's internals, the resolver is used for only mapping from a module name to a module filesystem path. And this mapping is 1:1 and also cached. The file content loaded from the mapped path is also cached in two layers of caching. All this happens outside of the resolver's code.

@cpojer
Copy link
Member

cpojer commented May 10, 2018

Maybe we can make some adjustments to enable use of a custom resolver for this.

@peter-leonov
Copy link

This would be the right modular way to go. Shell I try creating a proof of concept PR then?

@cpojer
Copy link
Member

cpojer commented May 10, 2018

Go for it!

@peter-leonov
Copy link

Here it comes with more text than code 😂
#6282

@peter-leonov
Copy link

@cpojer btw, if the resource queries topic is not so well fitting into Jest, maybe there is another way to hook into and get dynamic mocking possible? I'm super excited with what's new in Jest 23 and I'd be happy to see even more test automation (like we've done in Introscope) built into Jest 24 ;)

@peter-leonov
Copy link

Hey guys! The latest Jest release broke my naive monkey patches for the resource queries support. I'd still love to see some API like this to selectively (for performance reasons) hook into the transpiling phase for my little robust unit testing babel plugin.

Here are two micro-screencasts to show that I'm serious with this thing 😎

Manipulating module's scope:
inc

Snapshotting module's side effects:
abc

There is also a side effects recording / payback functionality and I have no idea how far we could get together if there were an API for tools like this in Jest.

@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 12, 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.

Update webpack tutorial on imports with query parameters
5 participants