Custom importers cause out of order chained imports.

[nowells]

I added a failing test case to demonstrate a bug in the current importers.

This error appears when you have custom importers defined, when a file has a chained import where the first import references a file not handled by the custom importer, and the second import is handled by the custom importer, the import order is broken.

[xzyfer]

Thanks for the test case @nowells. I've taken a quick look into it and it's certainly weird.

[xzyfer]

After some digging around the issue appears to be related to peculiar @import syntax you're using.
Splitting it into multiple imports resolves the issue.

@import "file-not-processed-by-loader";
@import "file-processed-by-loader";

My guess is this is a bug in how LibSass handles this kind of multiple import syntax.

[xzyfer]

With a little more digging it looks like LibSass is changing the order of imports.

Multiple imports

    .render(importer)
file-not-processed-by-loader
file-processed-by-loader
-------
/node-sass/test/fixtures/include-files/file-processed-by-loader.scss
####################################################################
Block 0x101501290 (0@[0:0]-[3:0]) [root] 0
 Import_Stub 0x101501f10 (0@[0:8]-[0:38]) [file-not-processed-by-loader.scss]  0
 Import_Stub 0x101008f20 (0@[1:8]-[1:34]) [/node-sass/test/fixtures/include-files/file-processed-by-loader.scss]  0
####################################################################

Single import

  api
    .render(importer)
file-not-processed-by-loader
file-processed-by-loader
-------
/node-sass/test/fixtures/include-files/file-processed-by-loader.scss
####################################################################
Block 0x10100c030 (0@[0:0]-[3:0]) [root] 0
 Import_Stub 0x101502900 (0@[2:40]-[2:66]) [/node-sass/test/fixtures/include-files/file-processed-by-loader.scss]  0
 Import_Stub 0x1015029e0 (0@[2:40]-[2:66]) [file-not-processed-by-loader.scss]  0
####################################################################

[nowells]

@xzyfer the bug is certainly in libsass, when I apply this diff and build my tests pass (but 2 media query tests fail, clearly this is not what we want to do, but wanted to show that the disconnect in the immediate call_importers and deferred to_import handling is what is causing our issue here)

diff --git a/src/parser.cpp b/src/parser.cpp
index 8e18e3d..83f4230 100644
--- a/src/parser.cpp
+++ b/src/parser.cpp
@@ -285,10 +285,12 @@ namespace Sass {
   Import* Parser::parse_import()
   {
     Import* imp = SASS_MEMORY_NEW(ctx.mem, Import, pstate);
-    std::vector<std::pair<std::string,Function_Call*>> to_import;
     bool first = true;
     do {
       while (lex< block_comment >());
+
+      std::vector<std::pair<std::string,Function_Call*>> to_import;
+
       if (lex< quoted_string >()) {
         if (!ctx.call_importers(unquote(std::string(lexed)), path, pstate, imp))
         {
@@ -322,6 +324,15 @@ namespace Sass {
         if (first) error("@import directive requires a url or quoted path", pstate);
         else error("expecting another url or quoted path in @import list", pstate);
       }
+
+      for(auto location : to_import) {
+        if (location.second) {
+          imp->urls().push_back(location.second);
+        } else {
+          ctx.import_url(imp, location.first, path);
+        }
+      }
+
       first = false;
     } while (lex_css< exactly<','> >());

@@ -330,14 +341,6 @@ namespace Sass {
       imp->media_queries(media_queries);
     }

-    for(auto location : to_import) {
-      if (location.second) {
-        imp->urls().push_back(location.second);
-      } else {
-        ctx.import_url(imp, location.first, path);
-      }
-    }
-
     return imp;
   }

I believe this is because we should really be having if (!ctx.call_importers(unquote(std::string(lexed)), path, pstate, imp)) follow the same logic and add their imports to the to_import array which is processed at the bottom after full parsing and resolution of media_queries, but instead the call_importers directly adds to the imp->urls() as the imports are discovered.

[xzyfer]

I came to the same conclusion last night. The correct patch is a little
more involved but not difficult. I'll push a PR up tonight.
On Feb 17, 2016 2:08 AM, "Nowell Strite" notifications@github.com wrote:

@xzyfer https://github.com/xzyfer the bug is certainly in libsass, when
I apply this diff and build my tests pass (but 2 media query tests fail)

diff --git a/src/parser.cpp b/src/parser.cpp
index 8e18e3d..83f4230 100644--- a/src/parser.cpp+++ b/src/parser.cpp@@ -285,10 +285,12 @@ namespace Sass {
Import* Parser::parse_import()
{
Import* imp = SASS_MEMORY_NEW(ctx.mem, Import, pstate);- std::vectorstd::pairstd::string,Function_Call* to_import;
bool first = true;
do {
while (lex< block_comment >());++ std::vectorstd::pairstd::string,Function_Call* to_import;+
if (lex< quoted_string >()) {
if (!ctx.call_importers(unquote(std::string(lexed)), path, pstate, imp))
{@@ -322,6 +324,15 @@ namespace Sass {
if (first) error("@import directive requires a url or quoted path", pstate);
else error("expecting another url or quoted path in @import list", pstate);
}++ for(auto location : to_import) {+ if (location.second) {+ imp->urls().push_back(location.second);+ } else {+ ctx.import_url(imp, location.first, path);+ }+ }+
first = false;
} while (lex_css< exactly<','> >());
@@ -330,14 +341,6 @@ namespace Sass {
imp->media_queries(media_queries);
}

• for(auto location : to_import) {- if (location.second) {- imp->urls().push_back(location.second);- } else {- ctx.import_url(imp, location.first, path);- }- }-
  return imp;
  }

I believe this is because we should really be having if
(!ctx.call_importers(unquote(std::string(lexed)), path, pstate, imp))
follow the same logic and add their imports to the to_import array which
is processed at the bottom after full parsing and resolution of
media_queries, but instead the call_importers directly adds to the
imp->urls() as the imports are discovered.

—
Reply to this email directly or view it on GitHub
#1381 (comment).

[nowells]

@xzyfer ❤️ 🎆 awesome! Thanks for spending the time to track this down! I look forward to seeing the proper fix so I can learn how to solve this properly myself next time around!

[xzyfer]

LibSass PR sass/libsass#1921

[xzyfer]

This test should pass now that #1416 has been merged.