-
Notifications
You must be signed in to change notification settings - Fork 204
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
Fix path transformation for dot files that are sibling to current file #253
Conversation
Codecov Report
|
Ping @tleunen |
Please address the comments I left so we can move forward with this PR. |
@fatfisz Maybe I'm looking in the wrong place, but I don't see any comments? |
That's strange, I'll try to make them visible to you too somehow (I can still see them). |
I don't see any comment neither @fatfisz. Did you forget to publish them? :) |
src/utils.js
Outdated
@@ -11,14 +11,20 @@ export function nodeResolvePath(modulePath, basedir, extensions) { | |||
} | |||
} | |||
|
|||
export function isRelativePath(nodePath) { | |||
return nodePath.match(/\.?\.\//); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be /^\.?\.\//
? Otherwise it will match foo./bar
too. Could you add a test for that just in case of a regression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch
test/index.test.js
Outdated
@@ -307,16 +307,16 @@ describe('module-resolver', () => { | |||
[plugin, { | |||
root: './test/fakepath/', | |||
alias: { | |||
constants: './test/testproject/src/constants', | |||
src: './test/testproject/src', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure, might have snuck in from another change, reverting.
test/index.test.js
Outdated
plugins: [ | ||
[plugin, { | ||
alias: { | ||
elintrc: './.eslintrc', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is ok, but could you add one more regression test in which the alias itself starts with a dot? I'm thinking of '.babel': 'babel-core'
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Wow, I didn't know that I had to do that. Sorry for the confusion 🙏 |
Looks great! I'll go ahead and merge. Thanks again for your contribution @amosyuen! |
Currently the code checks for a relative path by checking if it starts with a dot. This incorrectly identifies a path resolved dot file e.g.
.eslintrc
as a relative path. So it does NOT transform the path to relative path such as./.eslintric
. This is fixed by using a regex that checks for a./
or../
at the start of the path.