require.resolve with options.paths + require.resolve.paths #6471
Conversation
jeysal
force-pushed
the
require-resolve-with-paths
branch
from
56fc4b4
to
8741ee0
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
jeysal
force-pushed
the
require-resolve-with-paths
branch
4 times, most recently
from
b6b2c03
to
806fb97
Compare
jeysal
force-pushed
the
require-resolve-with-paths
branch
from
806fb97
to
eee104a
Compare
|
@SimenB @thymikee @rickhanlonii someone want to review this so I know whether it is going in the right direction? :) |
|
I think this looks pretty good 🙂 @thymikee thoughts? |
packages/jest-resolve/src/index.js
Outdated
| ); | ||
| if (module) return module; | ||
|
|
||
| // Throw an error if the module could not be found. `resolve.sync` |
There was a problem hiding this comment.
Can we add the number back? This is a part of a group of commits, would be nice not to lose the context
There was a problem hiding this comment.
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?
| ); | ||
| } | ||
|
|
||
| if (moduleName[0] === '.') { |
There was a problem hiding this comment.
.startsWith(0)? Whatever is faster on latest V8
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
When did V8 define string access via bracket notation? If it works on Node 6, then we're fine with it.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Here from a few years ago, so this should not cause any Node version issues.
packages/jest-resolve/src/index.js
Outdated
| @@ -108,13 +108,12 @@ class Resolver { | |||
| return null; | |||
| } | |||
|
|
|||
| resolveModule( | |||
| from: Path, | |||
| _resolveModuleFromDirIfExists( | |||
There was a problem hiding this comment.
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
force-pushed
the
require-resolve-with-paths
branch
2 times, most recently
from
4991024
to
c43c54f
Compare
jeysal
changed the title
|
Alright, I've addressed @thymikee's comments and finished up unit tests, changelog etc. |
jeysal
force-pushed
the
require-resolve-with-paths
branch
from
c43c54f
to
5b19604
Compare
|
Sorry to be a bit insistent here, it's just kind of frustrating to keep this dangling around for no apparent reason. |
There was a problem hiding this comment.
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
force-pushed
the
require-resolve-with-paths
branch
from
5b19604
to
be2948a
Compare
|
Thanks for the feedback! I rebased it on master and indeed had to fix the changelog entries. |
| ); | ||
| } | ||
|
|
||
| if (moduleName[0] === '.') { |
There was a problem hiding this comment.
When did V8 define string access via bracket notation? If it works on Node 6, then we're fine with it.
|
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](`jestjs/jest#6667)) - `[jest-runtime]` Support `require.resolve.paths` ([#​6471](`jestjs/jest#6471)) - `[jest-runtime]` Support `paths` option for `require.resolve` ([#​6471](`jestjs/jest#6471)) ##### Fixes - `[jest-runner]` Force parallel runs for watch mode, to avoid TTY freeze ([#​6647](`jestjs/jest#6647)) - `[jest-cli]` properly reprint resolver errors in watch mode ([#​6407](`jestjs/jest#6407)) - `[jest-cli]` Write configuration to stdout when the option was explicitly passed to Jest ([#​6447](`jestjs/jest#6447)) - `[jest-cli]` Fix regression on non-matching suites ([6657](`jestjs/jest#6657)) - `[jest-runtime]` Roll back `micromatch` version to prevent regression when matching files ([#​6661](`jestjs/jest#6661)) --- </details> --- This PR has been generated by [Renovate Bot](https://renovatebot.com).
|
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. |
Summary
Implement
require.resolvewithoptions.pathsand implement
require.resolve.paths.Closes #6206
Test plan
e2e tests added
jest-resolve unit test for
resolveModulepaths override addedTODO
jest-resolve _resolveModuleFromDirIfExistspublic for it is used byjest-runtime. @SimenB any thoughts on the implementation so far?