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

require.resolve with options.paths + require.resolve.paths #6471

Merged
merged 5 commits into from
Jul 10, 2018

Conversation

jeysal
Copy link
Contributor

@jeysal jeysal commented Jun 14, 2018

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
Copy link

codecov-io commented Jun 14, 2018

Codecov Report

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

Impacted file tree graph

@@            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 jeysal force-pushed the require-resolve-with-paths branch 4 times, most recently from b6b2c03 to 806fb97 Compare June 16, 2018 13:55
@jeysal jeysal force-pushed the require-resolve-with-paths branch from 806fb97 to eee104a Compare June 28, 2018 21:33
@jeysal
Copy link
Contributor Author

jeysal commented Jun 28, 2018

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

@SimenB
Copy link
Member

SimenB commented Jun 29, 2018

I think this looks pretty good 🙂

@thymikee thoughts?

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

);
if (module) return module;

// Throw an error if the module could not be found. `resolve.sync`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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] === '.') {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -108,13 +108,12 @@ class Resolver {
return null;
}

resolveModule(
from: Path,
_resolveModuleFromDirIfExists(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jeysal force-pushed the require-resolve-with-paths branch 2 times, most recently from 4991024 to c43c54f Compare July 1, 2018 13:21
@jeysal jeysal changed the title WIP require.resolve with options.paths require.resolve with options.paths + require.resolve.paths Jul 1, 2018
@jeysal
Copy link
Contributor Author

jeysal commented Jul 1, 2018

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

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

jeysal commented Jul 4, 2018

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

@jeysal
Copy link
Contributor Author

jeysal commented Jul 10, 2018

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?

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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

jeysal commented Jul 10, 2018

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

Copy link
Contributor

@mjesun mjesun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

);
}

if (moduleName[0] === '.') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@arcanis
Copy link
Contributor

arcanis commented Jul 10, 2018

Good for me 🙂

@mjesun mjesun merged commit 9497013 into jestjs:master Jul 10, 2018
@jeysal jeysal deleted the require-resolve-with-paths branch July 10, 2018 15:41
calebeby referenced this pull request in Pigmice2733/scouting-frontend Jul 11, 2018
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#&#8203;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 ([#&#8203;6667](`https://github.com/facebook/jest/pull/6667`))
- `[jest-runtime]` Support `require.resolve.paths` ([#&#8203;6471](`https://github.com/facebook/jest/pull/6471`))
- `[jest-runtime]` Support `paths` option for `require.resolve` ([#&#8203;6471](`https://github.com/facebook/jest/pull/6471`))
##### Fixes

- `[jest-runner]` Force parallel runs for watch mode, to avoid TTY freeze ([#&#8203;6647](`https://github.com/facebook/jest/pull/6647`))
- `[jest-cli]` properly reprint resolver errors in watch mode ([#&#8203;6407](`https://github.com/facebook/jest/pull/6407`))
- `[jest-cli]` Write configuration to stdout when the option was explicitly passed to Jest ([#&#8203;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 ([#&#8203;6661](`https://github.com/facebook/jest/pull/6661`))

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).
@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.

support require.resolve options
7 participants