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

Fix path transformation for dot files that are sibling to current file #253

Merged
merged 2 commits into from
Jan 16, 2018

Conversation

amosyuen
Copy link
Contributor

@amosyuen amosyuen commented Dec 22, 2017

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.

@codecov
Copy link

codecov bot commented Dec 22, 2017

Codecov Report

Merging #253 into master will not change coverage.
The diff coverage is 100%.

Impacted Files Coverage Δ
src/utils.js 100% <100%> (ø) ⬆️
src/resolvePath.js 100% <100%> (ø) ⬆️

@amosyuen
Copy link
Contributor Author

amosyuen commented Jan 8, 2018

Ping @tleunen

@fatfisz
Copy link
Contributor

fatfisz commented Jan 14, 2018

Please address the comments I left so we can move forward with this PR.

@fatfisz fatfisz self-requested a review January 14, 2018 01:40
@amosyuen
Copy link
Contributor Author

@fatfisz Maybe I'm looking in the wrong place, but I don't see any comments?

@fatfisz
Copy link
Contributor

fatfisz commented Jan 15, 2018

That's strange, I'll try to make them visible to you too somehow (I can still see them).

@tleunen
Copy link
Owner

tleunen commented Jan 15, 2018

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(/\.?\.\//);
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

@@ -307,16 +307,16 @@ describe('module-resolver', () => {
[plugin, {
root: './test/fakepath/',
alias: {
constants: './test/testproject/src/constants',
src: './test/testproject/src',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

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, might have snuck in from another change, reverting.

plugins: [
[plugin, {
alias: {
elintrc: './.eslintrc',
Copy link
Contributor

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'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@fatfisz
Copy link
Contributor

fatfisz commented Jan 15, 2018

Wow, I didn't know that I had to do that. Sorry for the confusion 🙏

@fatfisz
Copy link
Contributor

fatfisz commented Jan 16, 2018

Looks great! I'll go ahead and merge. Thanks again for your contribution @amosyuen!

@fatfisz fatfisz merged commit 7dc2da6 into tleunen:master Jan 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants