fix(jest-resolve): don't confuse directories with files#8912
fix(jest-resolve): don't confuse directories with files#8912SimenB merged 5 commits intojestjs:masterfrom
Conversation
SimenB
force-pushed
the
resolve-directory
branch
from
c7cacfb to
7bf052d
Compare
| const resolveTarget = path.resolve(basedir, target); | ||
| let resolveTarget = path.resolve(basedir, target); | ||
| if (target === '..' || target.endsWith('/')) { | ||
| resolveTarget += '/'; |
There was a problem hiding this comment.
this was lost in https://github.com/facebook/jest/pull/4325/files#diff-3887f79db6232183908265dcf84007acL60, introducing this bug
There was a problem hiding this comment.
not sure about the target.endsWith('/') but, what if the target is . instead of ./?
There was a problem hiding this comment.
good call. I added a test for it, but I don't think it's the correct fix either
There was a problem hiding this comment.
How about checking for directory first?
diff --git a/packages/jest-resolve/src/defaultResolver.ts b/packages/jest-resolve/src/defaultResolver.ts
index 40ded2c1f..131234b8c 100644
--- a/packages/jest-resolve/src/defaultResolver.ts
+++ b/packages/jest-resolve/src/defaultResolver.ts
@@ -60,10 +60,7 @@ function resolveSync(
if (REGEX_RELATIVE_IMPORT.test(target)) {
// resolve relative import
- let resolveTarget = path.resolve(basedir, target);
- if (target === '.' || target === '..' || target.endsWith('/')) {
- resolveTarget += '/';
- }
+ const resolveTarget = path.resolve(basedir, target);
const result = tryResolve(resolveTarget);
if (result) {
return result;
@@ -100,7 +97,7 @@ function resolveSync(
const dir = path.dirname(name);
let result;
if (isDirectory(dir)) {
- result = resolveAsFile(name) || resolveAsDirectory(name);
+ result = resolveAsDirectory(name) || resolveAsFile(name);
}
if (result) {
// Dereference symlinks to ensure we don't create a separateSeems to work, test are passing.
There was a problem hiding this comment.
yeah, that was my original fix, see 499e10a. But it's not that way upstream, which works correctly...
There was a problem hiding this comment.
Why does upstream not handle '.' though? 🤔
There was a problem hiding this comment.
hah, good catch. That's a bug there, I opened up a PR. But that means I'm actually happy with this implementation 😅
Codecov Report
@@ Coverage Diff @@
## master #8912 +/- ##
==========================================
- Coverage 65.04% 65.02% -0.02%
==========================================
Files 283 283
Lines 12160 12114 -46
Branches 3015 3000 -15
==========================================
- Hits 7909 7877 -32
+ Misses 3608 3598 -10
+ Partials 643 639 -4
Continue to review full report at Codecov.
|
SimenB
force-pushed
the
resolve-directory
branch
from
d9aaa1e to
240e0bf
Compare
| expect(resolved).toBe(require.resolve('../__mocks__/mockJsDependency.js')); | ||
| }); | ||
|
|
||
| it('does not confuse directories with files', () => { |
| const resolveTarget = path.resolve(basedir, target); | ||
| let resolveTarget = path.resolve(basedir, target); | ||
| if (target === '..' || target.endsWith('/')) { | ||
| resolveTarget += '/'; |
There was a problem hiding this comment.
Why does upstream not handle '.' though? 🤔
SimenB
force-pushed
the
resolve-directory
branch
from
240e0bf to
c4c2ad4
Compare
|
I looked into this again in light of #9520 - this bug is present in upstream |
|
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. |
5 participants
Summary
Fixes #8910
This might be an inherited bug from
resolve: https://github.com/browserify/resolve/blob/bd3f55188f0f3d1dac1e29c2f9a0eaa0878d4bd0/lib/sync.js#L69 which this code came from in #4315.EDIT: No, seems like
resolveworks correctlyTest plan
Test added