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

selector interpolation quoted and out of order in 3.3.3 #1947

Closed
lunelson opened this issue Mar 17, 2016 · 11 comments
Closed

selector interpolation quoted and out of order in 3.3.3 #1947

lunelson opened this issue Mar 17, 2016 · 11 comments

Comments

@lunelson
Copy link

In this gist, you can see expected corrected output; but currently with node-sass 3.5.0-beta.1 / libsass 3.3.3 I am getting selector interpolation like this for the same code (possible relations: #1633, #1925), which seems to be regression since 3.3.2:

"a".wrap-- {
  debug: 'hello';
}

"b".wrap-- {
  debug: 'hello';
}

"a-b".wrap-- {
  debug: 'hello';
}

"c".wrap-- {
  debug: 'hello';
}

"a-c".wrap-- {
  debug: 'hello';
}

"b-c".wrap-- {
  debug: 'hello';
}

"a-b-c".wrap-- {
  debug: 'hello';
}

"d".wrap-- {
  debug: 'hello';
}

"a-d".wrap-- {
  debug: 'hello';
}

"b-d".wrap-- {
  debug: 'hello';
}

"a-b-d".wrap-- {
  debug: 'hello';
}

"c-d".wrap-- {
  debug: 'hello';
}

"a-c-d".wrap-- {
  debug: 'hello';
}

"b-c-d".wrap-- {
  debug: 'hello';
}

"a-b-c-d".wrap-- {
  debug: 'hello';
}
@xzyfer
Copy link
Contributor

xzyfer commented Mar 17, 2016

This appears to be a combination of both #1901 and #1907. Will confirm.

@xzyfer
Copy link
Contributor

xzyfer commented Mar 17, 2016

I am unable to reproduce the out of order selectors on 3.3.3. However the broken quoting is reproducible.

@xzyfer
Copy link
Contributor

xzyfer commented Mar 17, 2016

The patch to #1907 doesn't appear to fix the quoting issue seen here. This is a different issue.

@xzyfer
Copy link
Contributor

xzyfer commented Mar 17, 2016

This is another instance of #{} no stripping quotes. Reduced test case

.a-#{quote('' + b)} {
  c: d;
}

xzyfer added a commit to xzyfer/sass-spec that referenced this issue Mar 17, 2016
@xzyfer
Copy link
Contributor

xzyfer commented Mar 17, 2016

Spec added sass/sass-spec#735

@xzyfer xzyfer added this to the 3.4 milestone Mar 17, 2016
@lunelson
Copy link
Author

I am unable to reproduce the out of order selectors on 3.3.3.

Yeah I got the out-of-order selectors with node-sass latest release (v3.5.0-beta.1), plugged in to my fork of sintaxi/terraform which plugs in to my fork of sintaxi/harpjs, and I also get the error also if I use this build system in sublime text, which seems to be pointing at the latest version of sassc; however if I clone and run sassc as of right now, I don't get the error. I also don't get it when I switch node-sass back to v3.4.2.

@lunelson
Copy link
Author

lunelson commented Apr 5, 2016

@xzyfer I think I've identified what's going on, there's a series of bugs at work: it's not a problem of #{} not stripping quotes as much as a problem of double-quotes resulting from the quote() function and causing havoc with #{}, particularly when combined with @extend

Re quote():

.test {
  /* should quote */
  out: inspect(quote(b));
  /* should not double quote */
  out: inspect(quote('' + b));
  /* should fail, because input is a list (separate bug) */
  out: inspect(quote(a b c));
  /* should still not double quote */
  out: inspect(quote('a b c'));
}
.test {
  /* should quote */
  out: "b";
  /* should not double quote */
  out: '"b"';
  /* should fail, because input is a list (separate bug) */
  out: "a b c";
  /* should still not double quote */
  out: '"a b c"'; }

...and when trying to interpolate that:

/* this is ok */
.foo--#{quote(bar)} { c: d; }
/* this is not ok */
.foo--#{quote('' + bar)} { c: d; }
/* this is just weird */
.foo--#{quote('' + bar)} { @extend %baz; }
%baz { c: d; }
/* this is ok */
.foo--bar {
  c: d; }

/* this is not ok */
.foo--"bar" {
  c: d; }

/* this is just weird */
"bar".foo-- {
  c: d; }

@mgreter
Copy link
Contributor

mgreter commented Apr 5, 2016

Fix might be quite simple?

--- "a/functions-c6921c4-left.cpp"
+++ "b/functions.cpp"
@@ -902,6 +902,10 @@ namespace Sass {
     BUILT_IN(sass_quote)
     {
       AST_Node* arg = env["$string"];
+      if (String_Quoted* qstr = dynamic_cast<String_Quoted*>(arg)) {
+        qstr->quote_mark('*');
+        return qstr;
+      }
       std::string str(quote(arg->to_string(ctx.c_options), String_Constant::double_quote()));
       String_Quoted* result = SASS_MEMORY_NEW(ctx.mem, String_Quoted, pstate, str);
       result->is_delayed(true);

Seems to fix the double quote problems and selector interpolations looks better too?

/* this is ok */
.foo--bar {
  c: d; }

/* this is not ok */
.foo--bar {
  c: d; }

/* this is just weird */
.foo--bar {
  c: d; }

@lunelson
Copy link
Author

lunelson commented Apr 5, 2016

@mgreter that sounds like what canonical sass would do, yes: don't quote it if it's already quoted

@mgreter
Copy link
Contributor

mgreter commented Apr 5, 2016

Can you check if #1979 fixes this issue?

@lunelson
Copy link
Author

lunelson commented Apr 5, 2016

Sorry @mgreter I didn't get to testing it but glad to see its closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants