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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Preserve mustache whitespace stripping information in AST and printer #951

Merged
merged 1 commit into from
Jul 4, 2019
Merged

Preserve mustache whitespace stripping information in AST and printer #951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 10 additions & 2 deletions packages/@glimmer/syntax/lib/builders.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ function buildMustache(
params?: AST.Expression[],
hash?: AST.Hash,
raw?: boolean,
loc?: AST.SourceLocation
loc?: AST.SourceLocation,
strip?: AST.StripFlags
): AST.MustacheStatement {
if (typeof path === 'string') {
path = buildPath(path);
Expand All @@ -27,6 +28,7 @@ function buildMustache(
hash: hash || buildHash([]),
escaped: !raw,
loc: buildLoc(loc || null),
strip: strip || { open: false, close: false },
};
}

Expand All @@ -36,7 +38,10 @@ function buildBlock(
hash: Option<AST.Hash>,
_defaultBlock: AST.PossiblyDeprecatedBlock,
_elseBlock?: Option<AST.PossiblyDeprecatedBlock>,
loc?: AST.SourceLocation
loc?: AST.SourceLocation,
openStrip?: AST.StripFlags,
inverseStrip?: AST.StripFlags,
closeStrip?: AST.StripFlags
): AST.BlockStatement {
let defaultBlock: AST.Block;
let elseBlock: Option<AST.Block> | undefined;
Expand Down Expand Up @@ -69,6 +74,9 @@ function buildBlock(
program: defaultBlock || null,
inverse: elseBlock || null,
loc: buildLoc(loc || null),
openStrip: openStrip || { open: false, close: false },
inverseStrip: inverseStrip || { open: false, close: false },
closeStrip: closeStrip || { open: false, close: false },
};
}

Expand Down
50 changes: 44 additions & 6 deletions packages/@glimmer/syntax/lib/generation/print.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,26 @@ export default function build(
}

function openBlock(block: AST.BlockStatement): string {
return ['{{#', pathParams(block), blockParams(block), '}}'].join('');
return compactJoin([
'{{',
block.openStrip.open ? '~' : null,
'#',
pathParams(block),
blockParams(block),
block.openStrip.close ? '~' : null,
'}}',
]);
}

function closeBlock(block: any): string {
return ['{{/', build(block.path, options), '}}'].join('');
function closeBlock(block: AST.BlockStatement): string {
return compactJoin([
'{{',
block.closeStrip.open ? '~' : null,
'/',
build(block.path, options),
block.closeStrip.close ? '~' : null,
'}}',
]);
}

const output: string[] = [];
Expand Down Expand Up @@ -144,7 +159,13 @@ export default function build(
case 'MustacheStatement':
{
output.push(
compactJoin([ast.escaped ? '{{' : '{{{', pathParams(ast), ast.escaped ? '}}' : '}}}'])
compactJoin([
ast.escaped ? '{{' : '{{{',
ast.strip.open ? '~' : null,
pathParams(ast),
ast.strip.close ? '~' : null,
ast.escaped ? '}}' : '}}}',
])
);
}
break;
Expand Down Expand Up @@ -174,7 +195,16 @@ export default function build(
const lines: string[] = [];

if (ast.chained) {
lines.push(['{{else ', pathParams(ast), '}}'].join(''));
lines.push(
compactJoin([
'{{',
ast.inverseStrip.open ? '~' : null,
'else ',
pathParams(ast),
ast.inverseStrip.close ? '~' : null,
'}}',
])
);
} else {
lines.push(openBlock(ast));
}
Expand All @@ -183,7 +213,15 @@ export default function build(

if (ast.inverse) {
if (!ast.inverse.chained) {
lines.push('{{else}}');
lines.push(
compactJoin([
'{{',
ast.inverseStrip.open ? '~' : null,
'else',
ast.inverseStrip.close ? '~' : null,
'}}',
])
);
}
lines.push(build(ast.inverse, options));
}
Expand Down
17 changes: 14 additions & 3 deletions packages/@glimmer/syntax/lib/parser/handlebars-node-visitors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,17 @@ export abstract class HandlebarsNodeVisitors extends Parser {
hash = addInElementHash(this.cursor(), hash, block.loc);
}

let node = b.block(path, params, hash, program, inverse, block.loc);
let node = b.block(
path,
params,
hash,
program,
inverse,
block.loc,
block.openStrip,
block.inverseStrip,
block.closeStrip
);

let parentProgram = this.currentElement();

Expand All @@ -106,7 +116,7 @@ export abstract class HandlebarsNodeVisitors extends Parser {
}

let mustache: AST.MustacheStatement;
let { escaped, loc } = rawMustache;
let { escaped, loc, strip } = rawMustache;

if (isLiteral(rawMustache.path)) {
mustache = {
Expand All @@ -116,12 +126,13 @@ export abstract class HandlebarsNodeVisitors extends Parser {
hash: b.hash(),
escaped,
loc,
strip,
};
} else {
let { path, params, hash } = acceptCallNodes(this, rawMustache as HBS.MustacheStatement & {
path: HBS.PathExpression;
});
mustache = b.mustache(path, params, hash, !escaped, loc);
mustache = b.mustache(path, params, hash, !escaped, loc, strip);
}

switch (tokenizer.state) {
Expand Down
4 changes: 4 additions & 0 deletions packages/@glimmer/syntax/lib/types/nodes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ export interface MustacheStatement extends BaseNode {
params: Expression[];
hash: Hash;
escaped: boolean;
strip: StripFlags;
}

export interface BlockStatement extends BaseNode {
Expand All @@ -109,6 +110,9 @@ export interface BlockStatement extends BaseNode {
hash: Hash;
program: Block;
inverse?: Option<Block>;
openStrip: StripFlags;
inverseStrip: StripFlags;
closeStrip: StripFlags;

// Glimmer extensions
chained?: boolean;
Expand Down
17 changes: 14 additions & 3 deletions packages/@glimmer/syntax/test/generation/print-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,19 @@ QUnit.module('[glimmer-syntax] Code generation - source -> source', function() {

// newlines after opening block
'{{#each}}\n <li> foo </li>\n{{/each}}',

// TODO: fix whitespace control in codemod mode
// '\n{{~#foo-bar~}} {{~/foo-bar~}} ',
].forEach(buildTest);

test('whitespace control is preserved', function(assert) {
let before = '\n{{~var~}} ';
let after = '{{~var~}}';

assert.equal(printTransform(before), after);
});

test('block whitespace control is preserved', function(assert) {
let before = '\n{{~#foo-bar~}} {{~else if x~}} {{~else~}} {{~/foo-bar~}} ';
let after = '{{~#foo-bar~}}{{~else if x~}}{{~else~}}{{~/foo-bar~}}';
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, why is the newline removed? We want to ensure that it isn't removed...

Copy link
Member

Choose a reason for hiding this comment

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

Would you mind working through this in a follow up? I'd like to merge now...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rwjblue The newline is removed by handlebars.js parser:

handlebars/compiler/base.js#L22-L23
handlebars/compiler/whitespace-control.js#L30-L35

We could add a preserveWhitespace option that would allow to return the AST with all whitespace information left intact.

Currently the only WhitespaceControl option is ignoreStandalone, which we use in the codemod mode.

Copy link
Member

Choose a reason for hiding this comment

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

Ya, that seems good to me. In the meantime, I think we can work around by adding back the whitespace that is stripped (at least in "codemod" mode).


assert.equal(printTransform(before), after);
});
});
91 changes: 82 additions & 9 deletions packages/@glimmer/syntax/test/parser-node-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,10 +281,30 @@ test('Tokenizer: MustacheStatement encountered in afterAttributeValueQuoted stat

test('Stripping - mustaches', function() {
let t = 'foo {{~content}} bar';
astEqual(t, b.program([b.text('foo'), b.mustache(b.path('content')), b.text(' bar')]));
astEqual(
t,
b.program([
b.text('foo'),
b.mustache(b.path('content'), undefined, undefined, undefined, undefined, {
open: true,
close: false,
}),
b.text(' bar'),
])
);

t = 'foo {{content~}} bar';
astEqual(t, b.program([b.text('foo '), b.mustache(b.path('content')), b.text('bar')]));
astEqual(
t,
b.program([
b.text('foo '),
b.mustache(b.path('content'), undefined, undefined, undefined, undefined, {
open: false,
close: true,
}),
b.text('bar'),
])
);
});

test('Stripping - blocks', function() {
Expand All @@ -293,7 +313,10 @@ test('Stripping - blocks', function() {
t,
b.program([
b.text('foo'),
b.block(b.path('wat'), [], b.hash(), b.blockItself()),
b.block(b.path('wat'), [], b.hash(), b.blockItself(), undefined, undefined, {
open: true,
close: false,
}),
b.text(' bar'),
])
);
Expand All @@ -303,7 +326,17 @@ test('Stripping - blocks', function() {
t,
b.program([
b.text('foo '),
b.block(b.path('wat'), [], b.hash(), b.blockItself()),
b.block(
b.path('wat'),
[],
b.hash(),
b.blockItself(),
undefined,
undefined,
undefined,
undefined,
{ open: false, close: true }
),
b.text('bar'),
])
);
Expand All @@ -314,31 +347,67 @@ test('Stripping - programs', function() {
astEqual(
t,
b.program([
b.block(b.path('wat'), [], b.hash(), b.blockItself([b.text('foo ')]), b.blockItself()),
b.block(
b.path('wat'),
[],
b.hash(),
b.blockItself([b.text('foo ')]),
b.blockItself(),
undefined,
{ open: false, close: true }
),
])
);

t = '{{#wat}} foo {{~else}}{{/wat}}';
astEqual(
t,
b.program([
b.block(b.path('wat'), [], b.hash(), b.blockItself([b.text(' foo')]), b.blockItself()),
b.block(
b.path('wat'),
[],
b.hash(),
b.blockItself([b.text(' foo')]),
b.blockItself(),
undefined,
undefined,
{ open: true, close: false }
),
])
);

t = '{{#wat}}{{else~}} foo {{/wat}}';
astEqual(
t,
b.program([
b.block(b.path('wat'), [], b.hash(), b.blockItself(), b.blockItself([b.text('foo ')])),
b.block(
b.path('wat'),
[],
b.hash(),
b.blockItself(),
b.blockItself([b.text('foo ')]),
undefined,
undefined,
{ open: false, close: true }
),
])
);

t = '{{#wat}}{{else}} foo {{~/wat}}';
astEqual(
t,
b.program([
b.block(b.path('wat'), [], b.hash(), b.blockItself(), b.blockItself([b.text(' foo')])),
b.block(
b.path('wat'),
[],
b.hash(),
b.blockItself(),
b.blockItself([b.text(' foo')]),
undefined,
undefined,
undefined,
{ open: true, close: false }
),
])
);
});
Expand All @@ -354,7 +423,11 @@ test('Stripping - removes unnecessary text nodes', function() {
[],
b.hash(),
b.blockItself([b.element('li', ['body', b.text(' foo ')])]),
null
null,
undefined,
{ open: false, close: true },
undefined,
{ open: true, close: false }
),
])
);
Expand Down