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

Restore includePaths logic #474

Merged
merged 1 commit into from
Apr 21, 2016
Merged

Conversation

xzyfer
Copy link
Collaborator

@xzyfer xzyfer commented Apr 21, 2016

I believe this was mistakenly removed in #353.

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

Fixes #469
Fixes #473

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
@eoneill
Copy link
Contributor

eoneill commented Apr 21, 2016

I think the first half of the fix for #473 is fine.

However, I don't think you want to add the file's parent onto the includePaths. From my perspective, #469 is a bug in the app code, which gulp-sass incorrectly supported because of this prior behavior.

Given directory structure...

build/all.scss
build/_variables.scss
build/skins/_greentur.scss

_greentur.scss should not be able to @import "variables" unless build/ is explicitly part of the includePaths. This import should be @import "../variables".

Here's a slightly more complex project that demonstrates the issue:

build/
├── foo/
│   ├── _partial.scss
│   └── index.scss
├── bar/
│   ├── _partial.scss
│   └── index.scss
└── baz/
    └── _baz.scss

with the following...

// [foo/index.scss]
@import "baz/baz";
// [bar/index.scss]
@import "baz/baz";
// [baz/_baz.scss]
@import "partial"; // this is the issue

Now, if we prepend the parent directory of the file being compiled, @import "partial" (inside of baz/_baz.scss) becomes non-deterministic. When I compile foo/index.scss, partial resolves to foo/_partial.scss. When I compile bar/index.scss, partial now resolves to bar/_partial.scss.

@xzyfer
Copy link
Collaborator Author

xzyfer commented Apr 21, 2016

I understand it's a bug but it's a bug people rely on and removing has
caused havoc.
On 22 Apr 2016 4:36 AM, "Eugene ONeill" notifications@github.com wrote:

I think the first half of the fix for #473
#473 is fine.

However, I don't think you want to add the file's parent onto the
includePaths. From my perspective, #469
#469 is a bug in the app
code, which gulp-sass incorrectly supported because of this prior behavior.

Given directory structure...

build/all.scss
build/_variables.scss
build/skins/_greentur.scss

_greentur.scss should not be able to @import "variables" unless build/ is
explicitly part of the includePaths. This import should be @import
"../variables".

Here's a slightly more complex project that demonstrates the issue:

build/
├── foo/
│ ├── _partial.scss
│ └── index.scss
├── bar/
│ ├── _partial.scss
│ └── index.scss
└── baz/
└── _baz.scss

with the following...

// [foo/index.scss]@import "baz/baz";

// [bar/index.scss]@import "baz/baz";

// [baz/_baz.scss]@import "partial"; // this is the issue

Now, if we prepend the parent directory of the file being compiled, @import
"partial" (inside of baz/_baz.scss) becomes non-deterministic. When I
compile foo/index.scss, partial resolves to foo/_partial.scss. When I
compile bar/index.scss, partial now resolves to bar/_partial.scss.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#474 (comment)

@xzyfer xzyfer merged commit f049f68 into dlmanning:master Apr 21, 2016
@xzyfer xzyfer deleted the feat/fix-include-paths branch April 21, 2016 22:52
@eoneill
Copy link
Contributor

eoneill commented Apr 21, 2016

👍

@nicolasartman
Copy link

I would love to see this fixed in a forthcoming version (since it's a breaking change, it'd have to be a major version bump I'm guessing). Sass, based on my reading, has moved to not automatically including the current (or parent) directory in the load path for a file, so the closer this can get to standard behavior the better. Right now I'm actually working around it by making weird folder structures so load paths don't end up unintentionally preceding each other

How would you feel about adding a flag for now that changes the behavior to the fixed version?

@xzyfer
Copy link
Collaborator Author

xzyfer commented Apr 22, 2016

How would you feel about adding a flag for now that changes the behavior to the fixed version?

Happy for you to PR this.

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