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

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

Open
ptrxyz opened this issue Apr 13, 2023 · 4 comments
Open

Comments

@ptrxyz
Copy link

ptrxyz commented Apr 13, 2023

See the code comments here: #356 and the corresponding line of code here:

/([^ \t\n])(([ \t]+$)|([ \t]+(\r?\n))|[ \t]+)/g,

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), isEndOfLine evaluates to `false`, and for that reason, in line

: characterBeforeWhitespace + (isEndOfLine ? endOfLine : ' '),
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
Copy link
Author

ptrxyz commented Apr 14, 2023

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
Copy link
Author

ptrxyz commented Apr 14, 2023

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
Copy link
Author

ptrxyz commented Apr 14, 2023

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));
}

@jc955
Copy link

jc955 commented May 8, 2024

Any progress on this issue?

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

No branches or pull requests

2 participants