require.resolve with options.paths + require.resolve.paths by jeysal · Pull Request #6471 · jestjs/jest

jeysal · 2018-06-14T22:46:40Z

Summary

Implement require.resolve with options.paths
and implement require.resolve.paths.
Closes #6206

Test plan

e2e tests added
jest-resolve unit test for resolveModule paths override added

TODO

update changelog
we probably need unit (non-e2e) tests for this as well
make jest-resolve _resolveModuleFromDirIfExists public for it is used by jest-runtime. @SimenB any thoughts on the implementation so far?

codecov-io · 2018-06-14T23:14:15Z

Codecov Report

Merging #6471 into master will decrease coverage by 0.14%.
The diff coverage is 80.76%.

@@            Coverage Diff             @@
##           master    #6471      +/-   ##
==========================================
- Coverage   63.73%   63.59%   -0.15%     
==========================================
  Files         235      235              
  Lines        8940     8966      +26     
  Branches        3        3              
==========================================
+ Hits         5698     5702       +4     
- Misses       3241     3263      +22     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-resolve/src/index.js	`96.06% <100%> (+0.09%)`	⬆️
packages/jest-runtime/src/index.js	`68.01% <72.22%> (-4.52%)`	⬇️

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 81d6888...be2948a. Read the comment docs.

jeysal · 2018-06-28T22:14:28Z

@SimenB @thymikee @rickhanlonii someone want to review this so I know whether it is going in the right direction? :)

SimenB · 2018-06-29T13:43:09Z

I think this looks pretty good 🙂

@thymikee thoughts?

thymikee

Looks OK, but I only had a short moment to check it on the phone

thymikee · 2018-06-29T14:30:44Z

packages/jest-resolve/src/index.js

+    );
+    if (module) return module;
+
+    // Throw an error if the module could not be found. `resolve.sync`


Can we add the number back? This is a part of a group of commits, would be nice not to lose the context

Not sure if that makes it more or less confusing - maybe (4.) to indicate that it is an "optional" 4th step, executed in the resolveModule code path but not if _resolveModuleFromDirIfExists is called directly?

thymikee · 2018-06-29T14:33:34Z

packages/jest-runtime/src/index.js

+      );
+    }
+
+    if (moduleName[0] === '.') {


.startsWith(0)? Whatever is faster on latest V8

Totally forgot that function exists, would be cleaner for sure.
Not sure if performance is that critical here (how often would a user call require.resolve.paths?) but I can try to find/run a benchmark later.

Okay so if you run it a lot, [0] === '.' is many times faster than .startsWith('.').
I think because of this and because the startsWith only really simplifies the code a lot when called with a multi character string, we can leave it this way.

When did V8 define string access via bracket notation? If it works on Node 6, then we're fine with it.

CI definitely passed on Node 6 last time.
I also think I've seen this being done somewhere else in the codebase already, let me check.

Here from a few years ago, so this should not cause any Node version issues.

jeysal · 2018-06-29T15:15:43Z

packages/jest-resolve/src/index.js


-  resolveModule(
-    from: Path,
+  _resolveModuleFromDirIfExists(


Can we make this public (remove the _) with this signature so we can properly rely on it in jest-runtime or does someone have concerns over the signature/naming?

jeysal · 2018-07-01T14:03:52Z

Alright, I've addressed @thymikee's comments and finished up unit tests, changelog etc.
Should be mergeable if CI passes.

jeysal · 2018-07-04T12:49:22Z

rebased on master.
@SimenB @thymikee mind merging this?
(or of course notifying me in case there's still something wrong)

jeysal · 2018-07-10T14:05:22Z

Sorry to be a bit insistent here, it's just kind of frustrating to keep this dangling around for no apparent reason.
@rickhanlonii @aaronabramov someone want to take a look if the others don't have time at the moment?

SimenB

Needs a quick rebase (changelog entries will end up in the wrong location as is, I think).

I think this is something @aaronabramov or @mjesun needs to look at (FB employees usually merge the larger changes)

… a module is missing

jeysal · 2018-07-10T14:30:23Z

Thanks for the feedback! I rebased it on master and indeed had to fix the changelog entries.

mjesun

LGTM. cc'ing @arcanis to double-check, since he's been working a lot in resolution logic.

mjesun · 2018-07-10T14:35:54Z

packages/jest-runtime/src/index.js

+      );
+    }
+
+    if (moduleName[0] === '.') {


When did V8 define string access via bracket notation? If it works on Node 6, then we're fine with it.

arcanis · 2018-07-10T15:32:53Z

Good for me 🙂

This Pull Request updates dependency [jest](https://github.com/facebook/jest) from `v23.3.0` to `v23.4.0` <details> <summary>Release Notes</summary> ### [`v23.4.0`](https://github.com/facebook/jest/blob/master/CHANGELOG.md#2340) [Compare Source](jestjs/jest@v23.3.0...v23.4.0) ##### Features - `[jest-haste-map]` Add `computeDependencies` flag to avoid opening files if not needed ([#6667](`https://github.com/facebook/jest/pull/6667`)) - `[jest-runtime]` Support `require.resolve.paths` ([#6471](`https://github.com/facebook/jest/pull/6471`)) - `[jest-runtime]` Support `paths` option for `require.resolve` ([#6471](`https://github.com/facebook/jest/pull/6471`)) ##### Fixes - `[jest-runner]` Force parallel runs for watch mode, to avoid TTY freeze ([#6647](`https://github.com/facebook/jest/pull/6647`)) - `[jest-cli]` properly reprint resolver errors in watch mode ([#6407](`https://github.com/facebook/jest/pull/6407`)) - `[jest-cli]` Write configuration to stdout when the option was explicitly passed to Jest ([#6447](`https://github.com/facebook/jest/pull/6447`)) - `[jest-cli]` Fix regression on non-matching suites ([6657](`https://github.com/facebook/jest/pull/6657`)) - `[jest-runtime]` Roll back `micromatch` version to prevent regression when matching files ([#6661](`https://github.com/facebook/jest/pull/6661`)) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com).

github-actions · 2021-05-12T11:07:28Z

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.

jeysal force-pushed the require-resolve-with-paths branch from 56fc4b4 to 8741ee0 Compare June 14, 2018 22:46

facebook-github-bot added the cla signed label Jun 14, 2018

jeysal force-pushed the require-resolve-with-paths branch 4 times, most recently from b6b2c03 to 806fb97 Compare June 16, 2018 13:55

jeysal force-pushed the require-resolve-with-paths branch from 806fb97 to eee104a Compare June 28, 2018 21:33

thymikee reviewed Jun 29, 2018

View reviewed changes

jeysal commented Jun 29, 2018

View reviewed changes

jeysal force-pushed the require-resolve-with-paths branch 2 times, most recently from 4991024 to c43c54f Compare July 1, 2018 13:21

jeysal changed the title ~~WIP require.resolve with options.paths~~ require.resolve with options.paths + require.resolve.paths Jul 1, 2018

jeysal force-pushed the require-resolve-with-paths branch from c43c54f to 5b19604 Compare July 4, 2018 11:39

SimenB approved these changes Jul 10, 2018

View reviewed changes

jeysal added 5 commits July 10, 2018 16:12

refactor(resolve): extract resolution function that does not throw if…

e3a7d79

… a module is missing

feat(resolve): add paths override to ResolveModuleConfig

ec79974

feat(runtime): support require.resolve with options.paths

3257c9a

test(resolve): resolveModule with custom paths

e669f89

test(module-name-mapper): update line number in snapshot

be2948a

jeysal force-pushed the require-resolve-with-paths branch from 5b19604 to be2948a Compare July 10, 2018 14:28

mjesun approved these changes Jul 10, 2018

View reviewed changes

mjesun merged commit 9497013 into jestjs:master Jul 10, 2018

jeysal deleted the require-resolve-with-paths branch July 10, 2018 15:41

ghost mentioned this pull request Nov 1, 2018

Update jest in / from 22.1.4 to 23.6.0 leonardofaria/leozera-ui#250

Closed

This was referenced Feb 2, 2020

Remove resolve package prettier/prettier#7508

Merged

require.resolve with options seems have different result in some case #9502

Closed

snyk-bot mentioned this pull request Mar 17, 2020

[Snyk] Upgrade expect from 23.0.0 to 23.6.0 yowainwright/react-workshop#8

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021

Uh oh!

Conversation

jeysal commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

TODO

Uh oh!

codecov-io commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jeysal commented Jun 28, 2018

Uh oh!

SimenB commented Jun 29, 2018

Uh oh!

thymikee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeysal commented Jul 1, 2018

Uh oh!

jeysal commented Jul 4, 2018

Uh oh!

jeysal commented Jul 10, 2018

Uh oh!

SimenB left a comment

Choose a reason for hiding this comment

Uh oh!

jeysal commented Jul 10, 2018

Uh oh!

mjesun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arcanis commented Jul 10, 2018

Uh oh!

github-actions bot commented May 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jeysal commented Jun 14, 2018 •

edited

Loading

codecov-io commented Jun 14, 2018 •

edited

Loading