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

add the .file property to options before handing off to node-sass #353

Merged
merged 3 commits into from
Mar 16, 2016

Conversation

eoneill
Copy link
Contributor

@eoneill eoneill commented Sep 30, 2015

This sets the file property on the options object that gets passed along to node-sass.

This is important for using eyeglass modules. eyeglass uses a custom importer and without knowing the file path origin, it isn't able to do it's job correctly.

Note that this doesn't really change how node-sass is going to render the file as data still takes precedence over file. This is just preserving the origin of the file.

Related issues:
linkedin/eyeglass#64
linkedin/eyeglass#42

Fixes #324

/cc @chriseppstein

@xzyfer
Copy link
Collaborator

xzyfer commented Oct 1, 2015

👍

@xzyfer
Copy link
Collaborator

xzyfer commented Oct 1, 2015

@eoneill can you please see if it's possible to create a test for this. The reason for this LOC is unclear so it's a prime candidate for being naively removed in the future.

@eoneill
Copy link
Contributor Author

eoneill commented Oct 2, 2015

So I've been trying to come up with a good test case for this, and discovered that we actually already test this functionality by virtue of any other test that does a relative import.

This also led me to discover that we can entirely remove the includePaths augmentation that we do.

The way the current logic works is that we take the path of the current file and add it's parent as the first path in includePaths.

This allows imports to seemingly work correctly, as the relative import fails, but it then finds the file in the parent path automagically added to includePaths.

By simply setting the file property, we can forgo touching includePaths and allow libsass to correctly resolve the paths for us.

I'm not sure what the correct test case is to represent all of this. If you have an idea what we should test, lemme know. I'll keep thinking about it in the meantime and hopefully come up with something.

@gvsboy
Copy link

gvsboy commented Oct 7, 2015

Many thanks for working on this PR! It solves our issue and makes Eyeglass a viable option in the world of Gulp. What is the status?

@eoneill
Copy link
Contributor Author

eoneill commented Oct 14, 2015

Let me know if you would like me to do anything else with this PR. Happy to make any changes/updates or discuss further.

@danreeves
Copy link

This would also close #368 and #324

@Keats
Copy link
Collaborator

Keats commented Dec 3, 2015

@eoneill re-ran the tests and it's currently failing

@eoneill
Copy link
Contributor Author

eoneill commented Dec 3, 2015

I'll take a look and update my PR if I can fix it.

@eoneill
Copy link
Contributor Author

eoneill commented Dec 3, 2015

Not sure what happened with the rebase, but my last commit (6c133e6) should fix the issues with sourcemaps.

@eoneill
Copy link
Contributor Author

eoneill commented Dec 4, 2015

Resolved rebase issues with --force. Should be good to review now.

@xzyfer
Copy link
Collaborator

xzyfer commented Dec 27, 2015

This seems reasonable to me.

@xzyfer
Copy link
Collaborator

xzyfer commented Dec 27, 2015

I'll happily release this in the new year. I want to deal with any issues that arise whilst I'm on holidays :)

@jefftherobot
Copy link

Can this be merged?

xzyfer added a commit that referenced this pull request Mar 16, 2016
Add the .file property to options before handing off to node-sass
@xzyfer xzyfer merged commit 74d6a1a into dlmanning:master Mar 16, 2016
@xzyfer
Copy link
Collaborator

xzyfer commented Mar 16, 2016

We're currently in a beta a release so this will appear in the next beta.

xzyfer added a commit to xzyfer/gulp-sass that referenced this pull request Apr 21, 2016
I believe this was mistakenly removed in dlmanning#353.

@eoneill can you shed some light on why you removed this? It has
caused issues for users.

Fixes dlmanning#469
Fixes dlmanning#473
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants