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

Resolve directory to the real path before using as base for resolve #7

Merged
merged 1 commit into from
Sep 23, 2017

Conversation

zkochan
Copy link
Contributor

@zkochan zkochan commented Jul 24, 2017

Close #6

index.js Outdated
@@ -11,7 +12,15 @@ const resolveFrom = (fromDir, moduleId, silent) => {
throw new TypeError(`Expected \`moduleId\` to be of type \`string\`, got \`${typeof moduleId}\``);
}

fromDir = path.resolve(fromDir);
try {
fromDir = resolveLinkTarget.sync(fromDir);
Copy link
Owner

Choose a reason for hiding this comment

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

Why not just fs.realpathSync()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Updated

index.js Outdated
try {
fromDir = fs.realpathSync(fromDir);
} catch (err) {
if (err.code !== 'EINVAL') {
Copy link
Owner

Choose a reason for hiding this comment

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

Not really sure why you're just throwing on EINVAL. Any error should be thrown.

You also have to correctly handle the silent argument. See line 32.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you're right, EINVAL was needed only when used with the resolve-link-target lib. All should be thrown when using fs.realpathSync. Also added tests for silent.

Thanks for the quick review. If anything else, I'll fix tomorrow. morning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sindresorhus, after thinking about this more, I am not so sure we have to throw any error. Right now it works if the fromDir does not exist in the filesystem. resolve seems to behave the same. So maybe we just need to use path.resolve on fromDir and then ignore ENOENT errors of realpathSync. What do you think?

@kevva
Copy link

kevva commented Jul 25, 2017

LGTM 👍

@sindresorhus sindresorhus changed the title fix: directories are resolved to real path before used as base for resolve Resolve directory to the real path before using as base for resolve Sep 23, 2017
@sindresorhus sindresorhus merged commit 749a248 into sindresorhus:master Sep 23, 2017
@sindresorhus
Copy link
Owner

Sorry, forgot about this. I'm gonna do a major bump just in case it affects something.

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