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

better support for builds that use symlink #172

Merged
merged 1 commit into from
Jun 27, 2017

Conversation

eoneill
Copy link
Contributor

@eoneill eoneill commented Jun 21, 2017

This issue was discovered while testing with broccoli 1.0. By default, broccoli will now use a global tmp path rather than local. This means that files get symlink'd in e.g. /var/folders/...

When eyeglass attempts to do it's module access check, it walks up the directory tree until it finds a package.json. It then uses that package.json to determine what dependencies can be imported.

When these files are symlink'd outside of the project workspace, eyeglass fails to find a package.json and thereby determines the origin scss file does not have access to the requested modules.

The fix here is to follow the symlink and check to see if the "real" file has access.

Note that we have to test both the real and symlink paths. We have to test the original symlink path because it's valid to have symlink'd files within a project and they should check access permission relative to the project before following the symlink.

Did not add tests because this is hard to test. Reduced test coverage thresholds a bit as such.

Copy link
Contributor

@chriseppstein chriseppstein left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks!

@eoneill eoneill merged commit 3126d85 into linkedin:master Jun 27, 2017
// if not...
if (!canAccess) {
// check for a symlink...
var realOrigin = fs.realpathSync(origin);
Copy link
Member

Choose a reason for hiding this comment

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

is there a reason not to always realpathSync ? node's resolution algorithm takes that approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

This could make a file that is linked to appear like it belongs to a particular module no longer appear as part of the module. It's possible that our validations are being done at the wrong time against the wrong values and that fixing the way those work would be the best solution -- but I think using realpath all the time implies more testing and code changes.

Copy link
Member

Choose a reason for hiding this comment

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

but I think using realpath all the time implies more testing and code changes.

Makes sense. I suspect this may be something todo, as it (from how I understand eyeglass to work) work more like the node require, and may lead to less surprises when symlinking. it's also possible I don't quite understand this part of eyeglass, and this doesn't make sense.

@stefanpenner
Copy link
Member

I don't believe this has been released yet, any reason to hold off?

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