-
Notifications
You must be signed in to change notification settings - Fork 465
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
Throwing error with a node-sass test on Windows #586
Comments
Still getting this error with latest master: http://pastebin.com/meJj5pfn. |
@am11 can you please provide the It'd be great if we could reproduce the error directly without node-sass. |
Hmm, that would be really difficult. I think stats is the node-sass only feature. It is implemented here: binding.cpp#L80-105. I am not sure if it is allocating memory incorrectly, or is it libsass' issue. But further tests revealed that the issue weirdly only happen when call from the test. From node-sass interactive shell, the same works just fine: C:\users\Adeel\Source\Repos\node-sass [master +1 ~1 -0 !]> cat C:\temp\foo.scss
// input
$test: (
one: 1,
two: 2,
);
$expect: (
two: 2,
one: 1,
);
.test {
equal: $test == $expect;
}
C:\users\Adeel\Source\Repos\node-sass [master +1 ~1 -0 !]> node
> var stats = {};
undefined
> require("./sass").renderSync({
... file: 'c:\\temp\\foo.scss',
... stats: stats,
... sourceMap: true
... });
'.test {\n equal: true; }\n\n/*# sourceMappingURL=undefined.map */'
> stats.sourceMap.indexOf('foo.scss') !== -1
true
> stats
{ entry: 'c:\\temp\\foo.scss',
start: 1414629548780,
includedFiles: [ 'c:/temp/foo.scss' ],
sourceMap: '{\n "version": 3,\n "file": "",\n "sources": ["..\\/..\\/..\\/..\\/..\\/..\\/c:\\/temp\\/foo.scss"],\n
"names": [],\n "mappings": "AAWA,AACE,AAAO"\n}',
end: 1414629548780,
duration: 0 } @kevva, can you provide any pointers for this? |
That |
Thanks for your help @am11 . Apologies for my naivety I'm not familiar with either the sourcemaps implementation or node-sass so I'm trying to track down where the issue maybe it. With this new information are you still certain the error was introduced in f55b2d5? Could it possibly be a sassc or node-sass issue? |
You don't have to apologize to be a C++ champ only! 😎 Till f55b2d5, it works fine: git checkout f55b2d50342f97228a56a513b4cabf40996d6092
# then remove existing binary build, and then
node build
# followed by
npm test it passes the stats test. The next commit is ca5f271, which concluded v3.0.1. When I So the issue seems to be in ca5f271. |
Hmm I annoyingly can't reproduce this on osx 👎 |
Oh please disregard my previous comment. It didn't removed the existing binaries. I have retested it and found the location of error. diff --git a/output_nested.cpp b/output_nested.cpp
index 6ead1bf..7ca38dc 100644
--- a/output_nested.cpp
+++ b/output_nested.cpp
@@ -262,7 +262,8 @@ namespace Sass {
void Output_Nested::append_to_buffer(const string& text)
{
buffer += text;
- if (ctx) ctx->source_map.update_column(text);
+ if (ctx && !ctx->_skip_source_map_update)
+ ctx->source_map.update_column(text);
} Using Apparently, it doesn't like |
Further diggings yielded; it's not the With this code: void Output_Nested::append_to_buffer(const string& text)
{
buffer += text;
// ctx->source_map.update_column(text);
} it throws the same exception, while this passes: void Output_Nested::append_to_buffer(const string& text)
{
buffer += text;
ctx->source_map.update_column(text);
} On x64 Windows with x86 node. On other x64 Windows with x64 node doesn't throw. |
I hope you don't mind, but I deleted the comment you said we can disregard . At least we now know that the issue is in the realm of the actual source map generating! I may can take a look at it tomorrow! But as @xzyfer already said, the source map generating is somehow a black box to us! IMO it rather looks like the call to |
@mgreter, yes that code was fine. Actually when the |
That makes perfect sense to delete that message. It constituted long lame outdated diffs anyway. :) |
Really great you could find the culprit, wasn't really in the mood the setup yet another compiler env :) Are you planning to add a PR to include appveyor for libsass? Because I think we could really benefit of a windows CI env!? This is somewhat on my todo list, but you may already got something working? |
Haven't made much progress there yet. Actually I cloned this repository https://github.com/darrenkopp/libsass-net by @darrenkopp. It contains a .NET wrapper of libsass. I am planning to get a VS compatible make file out of it (or vcxproj file), so it works with Appveyor. After that, we would have to do the same for sassc, to run the test. See sass/libsass-net@94abbd7#commitcomment-8333599. I think we can have a vcxpro file in both repository, so anyone with VS2013 can just open the file and click the green play button to build libsass! 😄 |
So lets shoutout to the |
Going by vcxproj file (notice |
I guess this can be closed since #591 has been merged! |
I verified that node-sass seems to be referencing a a commit after #591 was merged, but still running into issues. For example: $svg-fills: (
primary: $primary-color,
secondary: $secondary-color,
success: $success-color,
error: $error-color
);
@each $fill in $svg-fills {
.fill-#{$fill} { fill: map-get($svg-fills, $fill); }
} Throws an error Full error
When I reduce the above map to just two values, it works. Anything over that, and it breaks. $svg-fills: (
primary: $primary-color,
secondary: $secondary-color
); |
With new API, I get this error with your code: {
"status": 1,
"path": "c:/temp/foo3.scss",
"line": 2,
"column": 17,
"message": "unbound variable $primary-color"
} |
Hey, I run into the same issue with nearly the same code as @jpdesigndev. |
@mgreter, @xzyfer
When running this test in node-sass
should report correct sourceMap in stats with renderSync
on windows, it throws this error:I have tested further and it is working fine till this commit: f55b2d5. As soon as I try with the next commit (the last commit of v3.0.1): ca5f271, it throws the aforementioned error.
/cc @andrew
The text was updated successfully, but these errors were encountered: