-
Notifications
You must be signed in to change notification settings - Fork 381
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
Conversation
👍 |
@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. |
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 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 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 By simply setting the 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. |
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? |
Let me know if you would like me to do anything else with this PR. Happy to make any changes/updates or discuss further. |
@eoneill re-ran the tests and it's currently failing |
I'll take a look and update my PR if I can fix it. |
Not sure what happened with the rebase, but my last commit (6c133e6) should fix the issues with sourcemaps. |
Resolved rebase issues with |
This seems reasonable to me. |
I'll happily release this in the new year. I want to deal with any issues that arise whilst I'm on holidays :) |
Can this be merged? |
Add the .file property to options before handing off to node-sass
We're currently in a beta a release so this will appear in the next beta. |
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
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 overfile
. This is just preserving the origin of the file.Related issues:
linkedin/eyeglass#64
linkedin/eyeglass#42
Fixes #324
/cc @chriseppstein