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

Selectors with inline comments do not get a new line #1139

Closed
ntwb opened this issue Sep 12, 2015 · 6 comments
Closed

Selectors with inline comments do not get a new line #1139

ntwb opened this issue Sep 12, 2015 · 6 comments

Comments

@ntwb
Copy link

ntwb commented Sep 12, 2015

My setup on Mac OS X

$ ./node_modules/.bin/node-sass --version
node-sass   3.3.2   (Wrapper)   [JavaScript]
libsass     3.2.5   (Sass Compiler) [C/C++]

Example file file.scss:

button,
html input[type="button"], /* 1 */
input[type="reset"],
input[type="submit"] {
  -webkit-appearance: button; /* 2 */
  cursor: pointer; /* 3 */
}

Running ./node_modules/.bin/node-sass file.scss

Actual result:

button,
html input[type="button"], input[type="reset"],
input[type="submit"] {
  -webkit-appearance: button;
  /* 2 */
  cursor: pointer;
  /* 3 */ }

Expected result:

button,
html input[type="button"], 
input[type="reset"],
input[type="submit"] {
  -webkit-appearance: button;
  /* 2 */
  cursor: pointer;
  /* 3 */ }

Using sass v3.4.18 directly results in the expected result sass file.scss sass.css

button,
html input[type="button"],
input[type="reset"],
input[type="submit"] {
  -webkit-appearance: button;
  /* 2 */
  cursor: pointer;
  /* 3 */ }

/*# sourceMappingURL=sass.css.map */

The original fix I believe shipped with sass/libsass v3.2.0 here: sass/libsass#1007

It was also originally raised here in #826

Any ideas on a fix, I've spend quite a bit of time just now trying fix and hacking through the toubleshooting.md file trying to get the tests running locally to no avail :(

@saper
Copy link
Member

saper commented Sep 12, 2015

Good news:

Either the current libsass master I get:

button,
html input[type="button"],
input[type="reset"],
input[type="submit"] {
  -webkit-appearance: button;
  /* 2 */
  cursor: pointer;
  /* 3 */ }

So this will be fixed with the next release of libsass and the node-sass release that follows.

@saper saper added this to the next.libsass milestone Sep 12, 2015
@ntwb
Copy link
Author

ntwb commented Sep 12, 2015

Maybe I'm missing something but the upstream issue was fixed ~6 months ago and it should already be part of node-sass unless I'm missing something?

It was fixed in I believe in libsass 3.2.0 https://github.com/sass/libsass/releases/tag/3.2.0 via sass/libsass#1007 and this was released April 27 2015 and should have been included in node-sass v3.0.0 release https://github.com/sass/node-sass/releases/tag/v3.0.0 as node-sass v3.0.0 was the release that included the libsass 3.2.0 updates.

@saper
Copy link
Member

saper commented Sep 12, 2015

Did you check for your case with 3.2.0? Without digging in there are always those possibilities:

  1. both cases are subtly different bugs
  2. your case didn't work with 3.2.0 as well and the fix didn't fix the issue completely
  3. it got fixed in 3.2.0, then broke again in 3.2.5 (a regression) and got fixed later in the yet unreleased code. The tests we run should prevent this but it won't help in case of libsass is not compiled correctly by node-waf #1

If it's worth it with some effort one can identify commits that have fixed and/or re-introduced the issue.

@xzyfer
Copy link
Contributor

xzyfer commented Sep 14, 2015

@ntwb for the record node-sass@latest will always with run the latest stable LibSass at a minimum.

This was originally reported in sass/libsass#941 and thought to be fixed in 3.2. However due to a bug in our test suite we later realised it had only been partially fixed. The error was fixed but the white space was still incorrect. It was fixed for good in 3.3.

As @saper has said this is fixed on the current 3.3 beta and will be in node-sass when it's stable.

@xzyfer
Copy link
Contributor

xzyfer commented Sep 14, 2015

@ntwb you can track the LibSass 3.3 release progress by subscribing to sass/libsass#1453

@ntwb
Copy link
Author

ntwb commented Sep 15, 2015

Excellent, thanks for the additional feedback and clarification 👍

@xzyfer xzyfer modified the milestone: next.libsass Oct 25, 2015
jiongle1 pushed a commit to scantist-ossops-m2/node-sass that referenced this issue Apr 7, 2024
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

3 participants