[WIP] Replace postcss-scss with sass-parser #6
Conversation
There was a problem hiding this comment.
@nex3 @jamesnw This is still very WIP, but I'd appreciate some initial eyes on it to see if I'm going in the right direction. There's a lot of custom logic in the way Prettier currently handles values and comma-groups (everything in between two commas in a list) that I haven't really tackled yet -- I'm cautiously hopeful that we can ignore/remove a lot of it.
| } | ||
|
|
||
| node.selector = parseSelector(selector); | ||
| // TODO: node.selector cannot be overwritten | ||
| node.selectorTemp = parseSelector(selector); |
There was a problem hiding this comment.
This is still here because overriding node.selector raises an error, but postcss-selector-parser is still need until sass-parser exposes selector parsing.
| // TODO: Every node needs a `type` | ||
| node.type = "list"; |
There was a problem hiding this comment.
Prettier throws an error if any node is missing a type -- should sass-parser always set type internally?
There was a problem hiding this comment.
Does this mean that Prettier demands that every parser for every language conforms to a certain interface? That seems needlessly onerous.
The reason we don't set type for value-level nodes is that type represents the PostCSS notion of a node's type, rather than the Sass parser's more detailed notion. Since PostCSS doesn't have a notion of the type of expression-level constructs, we only provide sassType.
Honestly probably the easiest thing to do here would be to monkey-patch Expression.prototype to set a type property that just forwards to sassType if that's what PostCSS requires.
There was a problem hiding this comment.
I think we can tell Prettier to consistently use sassType for this parser 👍
| // TODO: It looks like `url()` is just parsed as a `string` | ||
| if ( | ||
| parentNode && | ||
| isURLFunctionNode(parentNode) && | ||
| (node.groups.length === 1 || | ||
| (node.groups.length > 0 && | ||
| node.groups[0].type === "value-comma_group" && | ||
| node.groups[0].groups.length > 0 && | ||
| node.groups[0].groups[0].type === "value-word" && | ||
| node.groups[0].groups[0].value.startsWith("data:"))) | ||
| ) { | ||
| return [hasParens ? "(" : "", join(",", nodes), hasParens ? ")" : ""]; | ||
| } |
There was a problem hiding this comment.
Prettier currently has special-handling for items inside a url() function, but it looks like sass-parser just treats urls as one big StringExpression.
There was a problem hiding this comment.
There are a number of function-esque expressions that Sass just parses as strings because they don't conform to the normal CSS expression-level conventions. For all intents and purposes in Sass these are string literals, and it's technically not safe to reformat them in most circumstances because even the whitespace is observable. The exception is URLs, where for historical reasons Sass will eliminate the leading and trailing whitespace. See this playground for an example.
There are also some cases where Sass parses URLs as actual Sass functions, but you won't need to worry about those since they'll have a different AST type entirely and can be handled by the normal function logic.
| // TODO: The original `shouldBreakList()` checks for comma-groups | ||
| node.some((node) => | ||
| ["list", "binary-operation"].includes(node.sassType), | ||
| ), |
There was a problem hiding this comment.
postcss-values-parser breaks every value between two commas into its own "comma-group" if it's more than a single node, and uses that to determine line-breaks and indentation. With sass-parser each item in a list is parsed individually, so this is an initial attempt to track a list of any sass-parser node types that postcss-values-parser would have considered more than one node (and therefore a "comma-group").
There was a problem hiding this comment.
Up to you in the end, but it's also possible that it's worth just rethinking the underlying logic of this formatting in the first place. Might be worth looking into how e.g. JS formats code in lists.
| @@ -123,24 +120,25 @@ function genericPrint(path, options, print) { | |||
| } | |||
|
|
|||
| return [ | |||
| node.raws.before.replaceAll(/[\s;]/gu, ""), | |||
| node.raws.before?.replaceAll(/[\s;]/gu, "") ?? "", | |||
There was a problem hiding this comment.
Because raws are not yet implemented in sass-parser, some inline comments are currently being inadvertently removed.
Notes
test.scssfile and runyarn debug test.scss-- you can then compare the output to the Prettier playground if useful.yarn test tests/format/scssDescription
Uses sass-parser for parsing Sass (instead of postcss-scss, postcss-media-query-parser, postcss-selector-parser, and postcss-values-parser).
Checklist
docs/directory).changelog_unreleased/*/XXXX.mdfile followingchangelog_unreleased/TEMPLATE.md.