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

feat: follow symlinks in dependency graph #76

Merged
merged 1 commit into from Oct 21, 2019

Conversation

keithamus
Copy link
Member

What?

This adds a call to require.resolve within dependency-graph, as require.resolve will canonicalize any symlinks within the path, to get the fully qualified path.

Why?

This allows us to use symlinks (such as npm link or yarn workspaces) and have the dependency graph resolve to the fully qualified files on disk. Without this change the dependency graph will only read the path, where the linter will lookup and resolve the existing files, and so there becomes a mismatch. Resolving symlinks resolves the mismatch.

We could optionally avoid using require.resolve and use fs.readlink - but fs.readlink will not canonicalize paths, in other words if node_modules/foo was a symlink then fs.readlink('node_modules/foo') would resolve, but fs.readlink('node_modules/foo/bar.js') would throw (other methods like fs.readFile('node_modules/foo/bar.js') will resolve as they do canonicalise symlinks).

@keithamus keithamus requested review from josh and a team October 17, 2019 10:20
Copy link
Contributor

@koddsson koddsson left a comment

Choose a reason for hiding this comment

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

This looks good to me but I'll leave the approval to someone else since we worked on this together.

@keithamus keithamus merged commit dc2f3e8 into master Oct 21, 2019
@keithamus keithamus deleted the feat-follow-symlinks-in-dependency-graph branch October 21, 2019 14:22
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