Skip to content
This repository has been archived by the owner on Jul 24, 2024. It is now read-only.

Importer returning contents and file no longer retain absolute paths #1219

Closed
gmac opened this issue Oct 26, 2015 · 7 comments
Closed

Importer returning contents and file no longer retain absolute paths #1219

gmac opened this issue Oct 26, 2015 · 7 comments

Comments

@gmac
Copy link

gmac commented Oct 26, 2015

There is a significant breaking API change in the way imported file contents reference subsequent imports between versions 3.3.3 and 3.4.0.

In v3.3.3, absolute file references provided with imported contents would be passed along for resolving subsequent imports. Example behavior in v3.3.3:

// @import "a"
done({
  file: '/Users/me/sass/lib/a.scss',
  contents: '@import "b"'
});

Next importer call for @import "b":

function(uri, previousUri, done) {
   // uri: "b",
   // previousUri: "/Users/me/sass/lib/a.scss"
}

In v3.4.0, absolute file references are apparently discarded, and only the previous slug is passed along to subsequent importer calls. Example behavior in v3.4.0:

// @import "a"
done({
  file: '/Users/me/sass/lib/a.scss',
  contents: '@import "b"'
});

Next importer call for @import "b":

function(uri, previousUri, done) {
   // uri: "b",
   // previousUri: "a"
}

This is a significant detriment to content importers that rely on the previously resolved content URI for file mapping, and a major enough change that I would have expected it to be part of a major version bump if intentional. If this change is intentional, I'd be curious about the rationale for removing this exceptionally valuable piece of data from the implementation. Without this data, importers now need to maintain a complex external dependency mapping table that can –at best– still only guess upon a previous slug's mapped file.

@gmac gmac changed the title Importer returning file and data no longer retains absolute file paths. Importer returning contents and file no longer retain absolute paths Oct 26, 2015
@mgreter
Copy link
Contributor

mgreter commented Oct 26, 2015

I guess https://github.com/sass/node-sass/blob/master/src/binding.cpp#L12 is the culprit, should probably be sass_import_get_imp_path. The names have changed in libsass API and it looks like this got mixed up on the node-sass side. Looks like node-sass could use some more unit tests.

@gmac
Copy link
Author

gmac commented Oct 26, 2015

Looks like node-sass could use some more unit tests. 👍

@mgreter
Copy link
Contributor

mgreter commented Oct 26, 2015

@gmac I guess their willing to accept Pull Requests in that direction 😉

@gmac
Copy link
Author

gmac commented Oct 26, 2015

Noted, and seriously considering.

@xzyfer
Copy link
Contributor

xzyfer commented Oct 27, 2015

This was fixed in #1220. Will land in v3.4.2.

jameslnewell pushed a commit to digitaledgeit/sass-composer that referenced this issue Oct 27, 2015
jameslnewell pushed a commit to jameslnewell/node-sass that referenced this issue Oct 27, 2015
xzyfer added a commit that referenced this issue Oct 27, 2015
@Rush
Copy link

Rush commented Oct 27, 2015

I am experiencing this problem as well, waiting for release then!

nfriedly added a commit to nfriedly/docpad-plugin-nodesass that referenced this issue Oct 28, 2015
sass/node-sass#1219 is killing me. Broken in v3.4.1 and expected to be fixed in v3.4.2
@xzyfer
Copy link
Contributor

xzyfer commented Nov 13, 2015

I believe this is fixed in v3.4.2. Please open if the issue persists.

@xzyfer xzyfer closed this as completed Nov 13, 2015
@xzyfer xzyfer modified the milestone: next.patch Sep 4, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants