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

Fix corrupt attribute values on dynamically bound Vue props without quotes #252

Merged
merged 5 commits into from
Jul 6, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/printer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -573,13 +573,16 @@ export class PugPrinter {
{ trimTrailingSemicolon = false }: FormatDelegatePrettierOptions = {}
): string {
val = val.trim();
val = val.slice(1, -1); // Remove quotes
const wasQuoted: boolean = isQuoted(val);
if (wasQuoted) {
val = val.slice(1, -1); // Remove quotes
}
val = format(val, { parser, ...this.codeInterpolationOptions });
val = unwrapLineFeeds(val);
if (trimTrailingSemicolon && val[val.length - 1] === ';') {
val = val.slice(0, -1);
}
return this.quoteString(val);
return wasQuoted ? this.quoteString(val) : val;
}

private formatStyleAttribute(val: string): string {
Expand Down
2 changes: 1 addition & 1 deletion src/utils/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ export function isWrappedWith(val: string, start: string, end: string, offset: n
* @returns Whether the value is quoted or not.
*/
export function isQuoted(val: string): boolean {
if (/^(["'])(.*)\1$/.test(val)) {
if (/^(["'`])(.*)\1$/.test(val)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I remember correctly, backtick quoted strings are pre processed by pug as JS. Therefore it is handled a bit differently than normal quoted strings.
But beside that, if this is the case, I should find a test case to cover that 🤔
Will have a deeper look later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bitencode Do you have a case for this? Then add it to the tests
If not, I would like to revert this line and just change it when it is absolutely necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Shinigami92 I do not have a specific test case for this. However, with the changes to avoid quoting pug variables assigned to Vue props to other test cases break (issue-61 - 'should not wrap chained data-bindings' and issue-80 - 'should not reformat multiline interpolation strings' because they are then not unquoted before being passed to the prettier format function.

It's possible that if isQuoted need to not check for back-tic quotes that there should be another function that does the check needed in formatDelegatePrettier. Or possibly an additional parameter that indicates which quotes to check for with the default being "' and the new call can pass in all 3 types of quotes. I don't know this code base well, or your coding preferences, so I'm open to suggestions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx 🙂 I will look into the branch and discover it
Will send feedback later back to you

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AHA! The thing is, that 61 and 80 have backtick quoted strings, but can be normal strings cause they don't use code interpolation! So no ${ in it.
Then my plugin converts these template literal strings to normal strings (by design)

But I currently don't understand why the new change is needed then 🤔

And we should create a test to check that it doesn't convert the backticks if it contains interpolated code in one single line

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bitencode So I added commit: a491e08

Seems that it still works with your change and not work when reverting...
So thx I'm now less afraid and confident using your change 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Awesome - Thanks!

// Regex for checking if there are any unescaped quotations.
const regex: RegExp = new RegExp(`${val[0]}(?<!\\\\${val[0]})`);
return !regex.test(val.slice(1, -1));
Expand Down
9 changes: 9 additions & 0 deletions tests/issues/issue-250/formatted.pug
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
- const circleSize = 72;
- const circleWidth = 7;
div
v-progress-circular(
:color="getProgressColor(currentCapacity)",
:size=circleSize,
:value="capacityPercentage",
:width=circleWidth
) {{ currentCapacity }}/{{ maxCapacity }}
14 changes: 14 additions & 0 deletions tests/issues/issue-250/issue-250.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import { readFileSync } from 'fs';
import { resolve } from 'path';
import { format } from 'prettier';
import { plugin } from './../../../src/index';

describe('Issues', () => {
test('should not quote pug variables assigned to Vue props', () => {
const expected: string = readFileSync(resolve(__dirname, 'formatted.pug'), 'utf8');
const code: string = readFileSync(resolve(__dirname, 'unformatted.pug'), 'utf8');
const actual: string = format(code, { parser: 'pug', plugins: [plugin] });

expect(actual).toBe(expected);
});
});
9 changes: 9 additions & 0 deletions tests/issues/issue-250/unformatted.pug
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
- const circleSize = 72;
- const circleWidth = 7;
div
v-progress-circular(
:color="getProgressColor(currentCapacity)",
:size=circleSize,
:value='capacityPercentage',
:width=circleWidth,
) {{ currentCapacity }}/{{ maxCapacity }}
5 changes: 5 additions & 0 deletions tests/issues/issue-61/formatted.pug
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,8 @@ div
a(
[href]='Nav.currentProject().repo(getRepository(pipeline).slug).buildNoContext()'
)
- const interpolation = "code"
//- Vue
a(:href=`interpolated-${interpolation}`)
//- Angular
a([href]=`interpolated-${interpolation}`)
5 changes: 5 additions & 0 deletions tests/issues/issue-61/unformatted.pug
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,8 @@ div
a(:href=`Nav.currentProject().repo(getRepository(pipeline).slug).buildNoContext()`)
//- Angular
a([href]=`Nav.currentProject().repo(getRepository(pipeline).slug).buildNoContext()`)
- const interpolation = 'code'
//- Vue
a(:href=`interpolated-${interpolation}`)
//- Angular
a([href]=`interpolated-${interpolation}`)
5 changes: 5 additions & 0 deletions tests/issues/issue-80/formatted.pug
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,8 @@ div
span.aui-lozenge(
:class='[isActive ? "some-class-name" : "some-other-class-name"]'
)

- const interpolation = "some-class-name"
span.aui-lozenge(
:class=`[ isActive ? ${interpolation} : "some-other-class-name" ]`
)
5 changes: 5 additions & 0 deletions tests/issues/issue-80/unformatted.pug
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,8 @@ div
span.aui-lozenge(
:class='[ isActive ? "some-class-name" : "some-other-class-name" ]'
)

- const interpolation = "some-class-name"
span.aui-lozenge(
:class=`[ isActive ? ${interpolation} : "some-other-class-name" ]`
)