sveltejs / prettier-plugin-svelte

Format your svelte components using prettier.
MIT License
735 stars 95 forks source link

white space of class attributes are not trimmed if at beginning of string #361

Open ptrxyz opened 1 year ago

ptrxyz commented 1 year ago

See the code comments here: https://github.com/sveltejs/prettier-plugin-svelte/pull/356 and the corresponding line of code here: https://github.com/sveltejs/prettier-plugin-svelte/blob/bd91bbf319fd9c469b041ba5901697de0e2229a3/src/print/index.ts#L194

White space at the beginning of the class attribute is not stripped when at the beginning of the string:

<div class="    foo     bar">

will turn into this:

<div class=" foo bar">

and not (as it should be):

<div class="foo bar">

The problem is: the first spaces are not captured by the regex, since match group 1 consumes the initial spaces and then match group two does not match anything anymore. So the first match we have is between foo and bar. We can fix this by extending the regexp to rawText = rawText.replace(/([^ \t\n]|^)(([ \t]+$)|([ \t]+(\r?\n))|[ \t]+)/g, now match group 1 will match the start of the string and the match group 2 will match the following leading spaces.

Then, we have to fix the replace function: for the first match (everything before foo), characterBeforeWhitespace evaluates to ` (empty string),isEndOfLineevaluates tofalse, and for that reason, in line https://github.com/sveltejs/prettier-plugin-svelte/blob/bd91bbf319fd9c469b041ba5901697de0e2229a3/src/print/index.ts#L207 we add a ` to the beginning of the string. I propose a fix by simply changing the two lines to this:

rawText = rawText.replace(/([^ \t\n]|^)(([ \t]+$)|([ \t]+(\r?\n))|[ \t]+)/g,
(match, characterBeforeWhitespace, _, isEndOfString, isEndOfLine, endOfLine) => isEndOfString
    ? match
    : characterBeforeWhitespace + (isEndOfLine ? endOfLine : characterBeforeWhitespace ? ' ' : ''));

so if characterBeforeWhitespace is empty, do not add the space character but instead do add nothing.

Example on regex101: https://regex101.com/r/xOuZbd/1

ptrxyz commented 1 year ago

Oh, I just find that the solution would not work with mustache expressions between different classes: foo {something} bar will turn into foo {something}bar.

Sadly I am not sure how to fix this, rawText seems to only hold the text after the {mustache} expression. No way to regex that I suppose.

ptrxyz commented 1 year ago

Maybe one thought on how to do this:

if (parent.type === 'Attribute') {
    // Direct child of attribute value -> add literallines at end of lines
    // so that other things don't break in unexpected places
    if (parent.name === 'class' && path.getParentNode(1).type === 'Element' || path.getParentNode(1).type === 'InlineComponent') {
        // Special treatment for class attribute on html elements. Prettier
        // will force everything into one line, we deviate from that and preserve lines.
        const afterMustacheTag = parent.value[parent.value.indexOf(node) - 1]?.type === "MustacheTag"
rawText = rawText.replace(/([^ \t\n]|^)(([ \t]+$)|([ \t]+(\r?\n))|[ \t]+)/g, 
        // Remove trailing whitespace in lines with non-whitespace characters
        // except at the end of the string
        (match, characterBeforeWhitespace, _, isEndOfString, isEndOfLine, endOfLine) => isEndOfString
            ? match
            : characterBeforeWhitespace + (isEndOfLine ? endOfLine : characterBeforeWhitespace || afterMustacheTag ? ' ' : ''));
        // Shrink trailing whitespace in case it's followed by a mustache tag
        // and remove it completely if it's at the end of the string, but not
        // if it's on its own line
        rawText = rawText.replace(/([^ \t\n])[ \t]+$/, parent.value.indexOf(node) === parent.value.length - 1 ? '$1' : '$1 ');
    }
    return concat(replaceEndOfLineWith(rawText, literalline));
}

Check if the previous node was a MoustacheTag, if so, do shrink all white spaces to a single one, otherwise do add nothing.

ptrxyz commented 1 year ago

Found one more thing just now: {mustache} will end up as {mustache} This is cause the 2nd regexp has the same problem as the first one before patching it, the fix is the same: rawText.replace(/([^ \t\n])[ \t]+$/ should be rawText.replace(/([^ \t\n]|^)[ \t]+$/

The full patch would then be this:

if (parent.type === 'Attribute') {
    // Direct child of attribute value -> add literallines at end of lines
    // so that other things don't break in unexpected places
    if (parent.name === 'class' && path.getParentNode(1).type === 'Element' || path.getParentNode(1).type === 'InlineComponent') {
        // Special treatment for class attribute on html elements. Prettier
        // will force everything into one line, we deviate from that and preserve lines.
        const afterMustacheTag = parent.value[parent.value.indexOf(node) - 1]?.type === "MustacheTag"
rawText = rawText.replace(/([^ \t\n]|^)(([ \t]+$)|([ \t]+(\r?\n))|[ \t]+)/g, 
        // Remove trailing whitespace in lines with non-whitespace characters
        // except at the end of the string
        (match, characterBeforeWhitespace, _, isEndOfString, isEndOfLine, endOfLine) => isEndOfString
            ? match
            : characterBeforeWhitespace + (isEndOfLine ? endOfLine : characterBeforeWhitespace || afterMustacheTag ? ' ' : ''));
        // Shrink trailing whitespace in case it's followed by a mustache tag
        // and remove it completely if it's at the end of the string, but not
        // if it's on its own line
        rawText = rawText.replace(/([^ \t\n]|^)[ \t]+$/, parent.value.indexOf(node) === parent.value.length - 1 ? '$1' : '$1 ');
    }
    return concat(replaceEndOfLineWith(rawText, literalline));
}
kk7771 commented 4 months ago

Any progress on this issue?