Fix parsing double slash comments inside Sass maps

[jsnajdr]

Which issue # if any, does this resolve? prettier/prettier#4659

When parsing a value that is a Sass map, and when the map contains some nested parens (nested map or function call, the tokenizer fails to recognize double slash comments:

(
  a: ( b: c )
  // comment
)

or

(
  a: calc( 1 + 2 )
  // comment
)

These comments are parsed as operator(/)+operator(/)+word instead.

Caused by the closeParen handler in tokenizer incorrectly setting isURLArg to true after a closing nested paren, even when there's no url() involved at all.

Introduced in #51 (@evilebottnawi), the PR that added support for Sass-like double-slash comments.

Please check one:

• New tests created for this change
• Tests updated for this change

---

[shellscape]

Please test this against the next branch.

[jsnajdr]

Please test this against the next branch.

The code in next is completely different and this patch cannot be applied there.

The next parser throws an "Unknown word" exception when trying to parse a Sass map value like (a:1).

[shellscape]

OK, that'll have to be accounted for on the next branch. I'll add it to the list.

[jsnajdr]

Hi @shellscape, what are the next steps on this PR? Is it ready to merge? Or does something need to happen on the next branch first? My goal would be to merge, release 2.0.1 and bump NPM dependency in Prettier. That would fix a Prettier bug I'm interested in having fixed.

[shellscape]

Current master is getting harder and harder to maintain, that's why I started on a complete rewrite. I've been delayed but planning on circling back to it this next week. If you can get CI passing for Node 10, or identify why it's failing, then I'll merge this and release a patch.

[jsnajdr]

If you can get CI passing for Node 10, or identify why it's failing, then I'll merge this and release a patch.

This is the stacktrace from the failing test run in CI:

internal/util/inspect.js:31
const types = internalBinding('types');
              ^
ReferenceError: internalBinding is not defined
    at internal/util/inspect.js:31:15
    at req_ (/home/travis/build/shellscape/postcss-values-parser/node_modules/natives/index.js:140:5)
    at require (/home/travis/build/shellscape/postcss-values-parser/node_modules/natives/index.js:113:12)
    at util.js:25:21
    at req_ (/home/travis/build/shellscape/postcss-values-parser/node_modules/natives/index.js:140:5)
    at require (/home/travis/build/shellscape/postcss-values-parser/node_modules/natives/index.js:113:12)
    at fs.js:42:21
    at req_ (/home/travis/build/shellscape/postcss-values-parser/node_modules/natives/index.js:140:5)
    at Object.req [as require] (/home/travis/build/shellscape/postcss-values-parser/node_modules/natives/index.js:54:10)
    at Object.<anonymous> (/home/travis/build/shellscape/postcss-values-parser/node_modules/vinyl-fs/node_modules/graceful-fs/fs.js:1:99)

Gulp depends on the natives module that tinkers with Node internals and is easy to break. Locally, the tests succeeded for me with 10.13.0, but I can reproduce the crash after upgrade to 10.15.0.

Upgrading natives from 1.1.5 to 1.1.6 in the lock file fixes the problem. (Pushed in 77c67ab) See also: gulpjs/gulp#2246

[jsnajdr]

Hello @shellscape! Are there still any blockers for this PR being merged? The Travis tests are fixed and passing, and the CircleCI integration doesn't seem to be set up.

[shellscape]

Cheers

[jsnajdr]

Thank you very much!