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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX] Process in-element in compiler not parser #1086

Merged
merged 2 commits into from
May 8, 2020
Merged

[BUGFIX] Process in-element in compiler not parser #1086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
46f881b
Make integration testing AST plugins possible
gitKrystan May 6, 2020
1b6a074
[BUGFIX] Process in-element in compiler not parser
gitKrystan May 6, 2020
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
70 changes: 57 additions & 13 deletions packages/@glimmer/compiler/lib/template-compiler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ function isTrustedValue(value: any) {
return value.escaped !== undefined && !value.escaped;
}

export const THIS = 0;
Copy link
Member

Choose a reason for hiding this comment

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

This was not used?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, it was dead code. Unless it’s some kind of public API but then since the entire codebase doesn’t do anything with it I don’t know what that would do either.


export default class TemplateCompiler implements Processor<InputOps> {
static compile(ast: AST.Template, source: string, options?: CompileOptions): Template {
let templateVisitor = new TemplateVisitor();
Expand Down Expand Up @@ -49,6 +47,12 @@ export default class TemplateCompiler implements Processor<InputOps> {
private locations: Option<SourceLocation>[] = [];
private includeMeta = true;

private cursorCount = 0;

cursor() {
return `%cursor:${this.cursorCount++}%`;
}

process(
actions: Action[]
): { opcodes: readonly Ops<AllocateSymbolsOps>[]; locations: readonly Option<SourceLocation>[] } {
Expand All @@ -62,6 +66,7 @@ export default class TemplateCompiler implements Processor<InputOps> {
}

startProgram([program]: [AST.Template]) {
this.cursorCount = 0;
Copy link
Member

Choose a reason for hiding this comment

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

I don't fully understand why we are resetting this counter for each program. I would have assumed that it was important that the guid we used was unique across each template (or is it needing to be more globally unique?), but resetting in startProgram here will lead to the same template reusing the same cursorCount (and therefore this.cursor() not being unique across the template).

D'oh! I just realized (after digging more into the code) that startProgram here is super misleading. It really should have been renamed to startTemplate when we split the usages of Program AST nodes into Template and Block.

I do still wonder what level of uniqueness we are looking for with the guid, but that is somewhat unrelated.

Copy link
Member

Choose a reason for hiding this comment

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

Somewhat ironically, the original code did suffer from the problem I described since it is operating on the Handlebars.Program (which is definitely emitted for each branch of a block).

See this AST Explorer showing that multiple Program nodes would have been seen in the HandlebarsNodeVisitors and therefore the same cursor would be reused within a template like

{{#if foo}}
  {{#in-element this.el}}{{/in-element}}
{{else}}
  {{#in-element this.el}}{{/in-element}}
{{/if}}

this.opcode(['startProgram', program], program);
}

Expand Down Expand Up @@ -242,7 +247,12 @@ export default class TemplateCompiler implements Processor<InputOps> {
}

block([action /*, index, count*/]: [AST.BlockStatement]) {
this.prepareHelper(action, 'block');
if (isInElement(action)) {
this.prepareHelper(action, 'in-element');
} else {
this.prepareHelper(action, 'block');
}

let templateId = this.templateIds.pop()!;
let inverseId = action.inverse === null ? null : this.templateIds.pop()!;
this.expression(action.path, ExpressionContext.BlockHead, action);
Expand Down Expand Up @@ -403,12 +413,12 @@ export default class TemplateCompiler implements Processor<InputOps> {
this.opcode(['helper'], call);
}

prepareHelper(expr: AST.Call, context: string) {
prepareHelper(expr: AST.Call, context: 'helper' | 'modifier' | 'block' | 'in-element') {
assertIsSimplePath(expr.path, expr.loc, context);

let { params, hash } = expr;

this.prepareHash(hash);
this.prepareHash(hash, context);
this.prepareParams(params);
}

Expand All @@ -428,23 +438,51 @@ export default class TemplateCompiler implements Processor<InputOps> {
this.opcode(['prepareArray', params.length], null);
}

prepareHash(hash: AST.Hash) {
prepareHash(hash: AST.Hash, context: 'helper' | 'modifier' | 'block' | 'in-element') {
let pairs = hash.pairs;
let length = pairs.length;

if (!pairs.length) {
this.opcode(['literal', null], null);
return;
}
let isInElement = context === 'in-element';
let hasInsertBefore = false;

for (let i = pairs.length - 1; i >= 0; i--) {
for (let i = length - 1; i >= 0; i--) {
let { key, value } = pairs[i];

if (isInElement) {
if (key === 'guid') {
throw new SyntaxError(
`Cannot pass \`guid\` to \`{{#in-element}}\` on line ${value.loc.start.line}.`,
value.loc
);
}

if (key === 'insertBefore') {
hasInsertBefore = true;
}
}

assert(this[value.type], `Unimplemented ${value.type} on TemplateCompiler`);
this[value.type](value as any);
this.opcode(['literal', key], null);
this.opcode(['literal', key]);
}

this.opcode(['prepareObject', pairs.length], null);
if (isInElement) {
if (!hasInsertBefore) {
this.opcode(['literal', undefined]);
this.opcode(['literal', 'insertBefore']);
length++;
}

this.opcode(['literal', this.cursor()]);
this.opcode(['literal', 'guid']);
length++;
}

if (length === 0) {
this.opcode(['literal', null]);
} else {
this.opcode(['prepareObject', length]);
}
}

prepareAttributeValue(value: AST.AttrNode['value']): value is AST.TextNode {
Expand Down Expand Up @@ -575,6 +613,12 @@ function isPath(node: AST.Node | AST.PathExpression): node is AST.PathExpression
return node.type === 'PathExpression';
}

function isInElement(
node: AST.BlockStatement
): node is AST.BlockStatement & { path: AST.PathExpression } {
return isPath(node.path) && node.path.original === 'in-element';
}

function destructureDynamicComponent(element: AST.ElementNode): Option<AST.PathExpression> {
let open = element.tag.charAt(0);

Expand Down
5 changes: 3 additions & 2 deletions packages/@glimmer/integration-tests/lib/compile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,10 @@ export const DEFAULT_TEST_META: AnnotatedModuleLocator = Object.freeze({
// out of the test environment.
export function preprocess(
template: string,
meta?: AnnotatedModuleLocator
meta?: AnnotatedModuleLocator,
options?: PrecompileOptions
): Template<AnnotatedModuleLocator> {
let wrapper = JSON.parse(rawPrecompile(template));
let wrapper = JSON.parse(rawPrecompile(template, options));
let factory = templateFactory<AnnotatedModuleLocator>(wrapper);
return factory.create(meta || DEFAULT_TEST_META);
}
Expand Down
26 changes: 23 additions & 3 deletions packages/@glimmer/integration-tests/lib/modes/aot/delegate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { PrecompileOptions } from '@glimmer/compiler';
import {
BundleCompiler,
DebugConstants,
Expand Down Expand Up @@ -31,6 +32,7 @@ import {
renderSync,
AotRuntime,
} from '@glimmer/runtime';
import { ASTPluginBuilder } from '@glimmer/syntax';
import { assert, assign, expect, Option } from '@glimmer/util';
import {
SimpleElement,
Expand Down Expand Up @@ -99,6 +101,7 @@ export class AotRenderDelegate implements RenderDelegate {
static readonly isEager = true;
static style = 'aot';

private plugins: ASTPluginBuilder[] = [];
protected registry = new AotCompilerRegistry();
protected compileTimeModules = new Modules();
protected symbolTables = new ModuleLocatorMap<ProgramSymbolTable, ModuleLocator>();
Expand Down Expand Up @@ -134,6 +137,10 @@ export class AotRenderDelegate implements RenderDelegate {
return this.doc.createDocumentFragment();
}

registerPlugin(plugin: ASTPluginBuilder): void {
this.plugins.push(plugin);
}

registerComponent(
type: ComponentKind,
testType: ComponentKind,
Expand Down Expand Up @@ -182,7 +189,10 @@ export class AotRenderDelegate implements RenderDelegate {
}

registerPartial(name: string, source: string) {
let definition = new PartialDefinitionImpl(name, preprocess(source));
let definition = new PartialDefinitionImpl(
name,
preprocess(source, undefined, this.precompileOptions)
);
this.registry.register(name, 'partial', { default: definition });
}

Expand All @@ -192,6 +202,14 @@ export class AotRenderDelegate implements RenderDelegate {
this.registry.register(name, 'modifier', { default: { manager, state } });
}

private get precompileOptions(): PrecompileOptions {
return {
plugins: {
ast: this.plugins,
},
};
}

private addRegisteredComponents(bundleCompiler: BundleCompiler): void {
let { registry, compileTimeModules } = this;
Object.keys(registry.components).forEach(key => {
Expand Down Expand Up @@ -256,7 +274,7 @@ export class AotRenderDelegate implements RenderDelegate {
}

private getBundleCompiler(): BundleCompiler {
let { compiler, constants } = getBundleCompiler(this.registry);
let { compiler, constants } = getBundleCompiler(this.registry, this.plugins);
this.constants = constants;

return compiler;
Expand Down Expand Up @@ -320,13 +338,15 @@ export class AotRenderDelegate implements RenderDelegate {
}

function getBundleCompiler(
registry: AotCompilerRegistry
registry: AotCompilerRegistry,
plugins?: ASTPluginBuilder[]
): { compiler: BundleCompiler; constants: DebugConstants } {
let delegate: AotCompilerDelegate = new AotCompilerDelegate(registry);
let constants = new DebugConstants();
let compiler = new BundleCompiler(delegate, {
macros: new TestMacros(),
constants,
plugins,
});
return { constants, compiler };
}
20 changes: 18 additions & 2 deletions packages/@glimmer/integration-tests/lib/modes/jit/delegate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { PrecompileOptions } from '@glimmer/compiler';
import {
JitRuntimeContext,
SyntaxCompilationContext,
Expand Down Expand Up @@ -36,6 +37,7 @@ import {
registerTemplate,
componentHelper,
} from './register';
import { ASTPluginBuilder } from '@glimmer/syntax';
import { TestMacros } from '../../compile/macros';
import JitCompileTimeLookup from './compilation-context';
import TestJitRuntimeResolver from './resolver';
Expand Down Expand Up @@ -74,6 +76,7 @@ export class JitRenderDelegate implements RenderDelegate {
static readonly isEager = false;
static style = 'jit';

private plugins: ASTPluginBuilder[] = [];
private resolver: TestJitRuntimeResolver = new TestJitRuntimeResolver();
private registry: TestJitRegistry = this.resolver.registry;
private context: JitTestDelegateContext;
Expand Down Expand Up @@ -119,6 +122,10 @@ export class JitRenderDelegate implements RenderDelegate {
return componentHelper(this.resolver, this.registry, name);
}

registerPlugin(plugin: ASTPluginBuilder): void {
this.plugins.push(plugin);
}

registerComponent<K extends 'Basic' | 'Fragment' | 'Glimmer', L extends ComponentKind>(
type: K,
_testType: L,
Expand Down Expand Up @@ -200,7 +207,7 @@ export class JitRenderDelegate implements RenderDelegate {
}

compileTemplate(template: string): HandleResult {
let compiled = preprocess(template);
let compiled = preprocess(template, undefined, this.precompileOptions);

return unwrapTemplate(compiled)
.asLayout()
Expand All @@ -214,9 +221,18 @@ export class JitRenderDelegate implements RenderDelegate {
template,
this.context,
this.getSelf(context),
this.getElementBuilder(this.context.runtime.env, cursor)
this.getElementBuilder(this.context.runtime.env, cursor),
this.precompileOptions
);
}

private get precompileOptions(): PrecompileOptions {
return {
plugins: {
ast: this.plugins,
},
};
}
}

function isBrowserTestDocument(doc: SimpleDocument): doc is SimpleDocument & Document {
Expand Down
6 changes: 4 additions & 2 deletions packages/@glimmer/integration-tests/lib/modes/jit/render.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { JitTestDelegateContext } from './delegate';
import { PrecompileOptions } from '@glimmer/compiler';
import { VersionedPathReference } from '@glimmer/reference';
import { ElementBuilder, RenderResult } from '@glimmer/interfaces';
import { preprocess } from '../../compile';
Expand All @@ -9,9 +10,10 @@ export function renderTemplate(
src: string,
{ runtime, syntax }: JitTestDelegateContext,
self: VersionedPathReference,
builder: ElementBuilder
builder: ElementBuilder,
options?: PrecompileOptions
): RenderResult {
let template = preprocess(src);
let template = preprocess(src, undefined, options);
let handle = unwrapTemplate(template)
.asLayout()
.compile(syntax);
Expand Down
27 changes: 25 additions & 2 deletions packages/@glimmer/integration-tests/lib/modes/rehydration/delegate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { PrecompileOptions } from '@glimmer/compiler';
import {
Cursor,
Dict,
Expand All @@ -8,6 +9,7 @@ import {
Helper,
} from '@glimmer/interfaces';
import { serializeBuilder } from '@glimmer/node';
import { ASTPluginBuilder } from '@glimmer/syntax';
import createHTMLDocument from '@simple-dom/document';
import {
SimpleDocument,
Expand Down Expand Up @@ -44,6 +46,8 @@ export class RehydrationDelegate implements RenderDelegate {
static readonly isEager = false;
static readonly style = 'rehydration';

private plugins: ASTPluginBuilder[] = [];

public clientEnv: JitTestDelegateContext;
public serverEnv: JitTestDelegateContext;

Expand Down Expand Up @@ -113,7 +117,8 @@ export class RehydrationDelegate implements RenderDelegate {
template,
this.serverEnv,
this.getSelf(context),
this.getElementBuilder(this.serverEnv.runtime.env, cursor)
this.getElementBuilder(this.serverEnv.runtime.env, cursor),
this.precompileOptions
);

takeSnapshot();
Expand All @@ -139,7 +144,13 @@ export class RehydrationDelegate implements RenderDelegate {
// Client-side rehydration
let cursor = { element, nextSibling: null };
let builder = this.getElementBuilder(env, cursor) as DebugRehydrationBuilder;
let result = renderTemplate(template, this.clientEnv, this.getSelf(context), builder);
let result = renderTemplate(
template,
this.clientEnv,
this.getSelf(context),
builder,
this.precompileOptions
);

this.rehydrationStats = {
clearedNodes: builder['clearedNodes'],
Expand All @@ -161,6 +172,10 @@ export class RehydrationDelegate implements RenderDelegate {
return this.renderClientSide(template, context, element);
}

registerPlugin(plugin: ASTPluginBuilder): void {
this.plugins.push(plugin);
}

registerComponent(type: ComponentKind, _testType: string, name: string, layout: string): void {
registerComponent(this.clientRegistry, type, name, layout);
registerComponent(this.serverRegistry, type, name, layout);
Expand All @@ -185,6 +200,14 @@ export class RehydrationDelegate implements RenderDelegate {
registerModifier(this.clientRegistry, name, ModifierClass);
registerModifier(this.serverRegistry, name, ModifierClass);
}

private get precompileOptions(): PrecompileOptions {
return {
plugins: {
ast: this.plugins,
},
};
}
}

export function qunitFixture(): SimpleElement {
Expand Down
2 changes: 2 additions & 0 deletions packages/@glimmer/integration-tests/lib/render-delegate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
SimpleDocumentFragment,
SimpleDocument,
} from '@simple-dom/interface';
import { ASTPluginBuilder } from '@glimmer/syntax';
import { ComponentKind, ComponentTypes } from './components';
import { UserHelper } from './helpers';
import {
Expand Down Expand Up @@ -39,6 +40,7 @@ export default interface RenderDelegate {
layout: string,
Class?: ComponentTypes[K]
): void;
registerPlugin(plugin: ASTPluginBuilder): void;
registerHelper(name: string, helper: UserHelper): void;
registerInternalHelper(name: string, helper: Helper): void;
registerPartial(name: string, content: string): void;
Expand Down
Loading