Template compiler

[bigopon]

@EisenbergEffect @fkleuver

The template compiler now understands custom elements and template controllers, but I'm unable to create an integration test for custom element. Any pointers would be nice.

aurelia/packages/jit/test/unit/templating/template-compiler.integration.spec.ts

Lines 505 to 524 in 8ce5f90

	it(`custom elements`, () => {
	@customElement({
	name: 'name-tag',
	templateOrNode: '<template>${name}</template>'
	})
	class NameTag {
	@bindable()
	name: string;
	}
	component = createCustomElement(
	`<template><name-tag name="bigopon"></name-tag></template>`,
	NameTag
	);
	debugger;
	// (component.constructor as any).description.dependencies.push(NameTag);
	au.app({ host, component: component }).start();
	cs.flushChanges();
	expect(host.textContent).to.equal('bigopon');
	});

@fkleuver I've commented parseAttribute() tests. I think I will need to rework them a bit as now compiling attribute has been changed a bit.

[EisenbergEffect]

@bigopon Do you need advice on setting up the test or with the implementation of the compiler itself?

[fkleuver]

Small nit: this could be a loose function instead of a private method

[fkleuver]

new BindingInstruction[bindingCommand & BindingType.Command] :P

[bigopon]

@EisenbergEffect I think I'm having issue with dependencies not picked up, as in this block

aurelia/packages/jit/test/unit/templating/template-compiler.integration.spec.ts

Lines 63 to 72 in 8ce5f90

	function createCustomElement(markup: string, ...dependencies: Function[]): { [key: string]: any } {
	return new (CustomElementResource.define({
	name: 'app',
	dependencies: [...dependencies],
	templateOrNode: markup,
	build: { required: true, compiler: 'default' },
	instructions: [],
	surrogates: []
	}, class App { }))();
	}

Is that the correct way to declare dependencies for a custom element ? I'm having empty instructions list inside app custom element description

[bigopon]

@EisenbergEffect in unit test, it works fine

aurelia/packages/jit/test/unit/templating/template-compiler.spec.ts

Lines 454 to 478 in f8ef773

	it('distinguishs element properties / normal attributes', () => {
	@customElement('el')
	class El {
	@bindable()
	name: string;
	}
	const actual = compileWith(
	`<template>
	<el name="name" name2="label"></el>
	</template>`,
	[El]
	);
	const rootInstructions = actual.instructions[0] as any[];
	const expectedRootInstructions = [
	{ toVerify: ['type', 'res'], type: TargetedInstructionType.hydrateElement, res: 'el' }
	];
	verifyInstructions(rootInstructions, expectedRootInstructions);
	const expectedElInstructions = [
	{ toVerify: ['type', 'mode', 'dest'], type: TargetedInstructionType.propertyBinding, mode: BindingMode.toView, dest: 'name' },
	];
	verifyInstructions(rootInstructions[0].instructions, expectedElInstructions);
	});

[EisenbergEffect]

I understand toLowerCase here but I'm not sure I understand why you need to hyphenate the tagName.

[bigopon]

Yes, it should be gone

[fkleuver]

res needs to be the registered name of the resource (in the case of your failing integration test "name-tag"). Then it works :)

(for simplicity sake: replace resources with node.tagName.toLowerCase())

[bigopon]

Wow, doh.!

[fkleuver]

This needs to be:

@customElement({
  name: 'name-tag',
  templateOrNode: '<template>${name}</template>',
  build: { required: true, compiler: 'default' },
  dependencies: [],
  instructions: [],
  surrogates: []
})

[fkleuver]

Actually just put this whole thing at the top of the file:

@customElement({
  name: 'name-tag',
  templateOrNode: '<template>asdf</template>',
  build: { required: true, compiler: 'default' },
  dependencies: [],
  instructions: [],
  surrogates: []
})
export class NameTag {
  @bindable()
  name: string;
}

And add it to globalresources:

const globalResources: any[] = [
  If,
  Else,
  Repeat,
  SortValueConverter,
  JsonValueConverter,
  OneTimeBindingBehavior,
  ToViewBindingBehavior,
  FromViewBindingBehavior,
  TwoWayBindingBehavior,
  DebounceBindingBehavior,
  ThrottleBindingBehavior,
  NameTag
];

And the test to this:

  it.only(`custom elements`, () => {
    component = createCustomElement(`<template><name-tag name="bigopon"></name-tag></template>`);
    au.app({ host, component: component })
    au.start();
    cs.flushChanges();
    expect(host.innerText).to.equal('bigopon');
  });

That will give a bindable error, but if you remove the bindable and instead put plain text in the custom element it will work. Not sure yet what that error is about

[fkleuver]

Maybe you just need to change this to Registration.transient('name-tag', NameTag). The dependency would get autoregistered by the function instead of the custom element name otherwise

[codeclimate]

Code Climate has analyzed commit f93f3f3 and detected 10 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	10

The test coverage on the diff in this pull request is 86.9% (50% is the threshold).

This pull request will bring the total coverage in the repository to 82.2% (0.9% change).

View more on Code Climate.

[codecov]

Codecov Report

Merging #121 into master will increase coverage by 0.82%.
The diff coverage is 85.82%.

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
+ Coverage   80.42%   81.25%   +0.82%     
==========================================
  Files          72       73       +1     
  Lines        5385     5467      +82     
  Branches      944      968      +24     
==========================================
+ Hits         4331     4442     +111     
+ Misses       1054     1025      -29

Impacted Files	Coverage Δ
...ckages/runtime/src/templating/template-compiler.ts	100% <ø> (ø)	⬆️
...ckages/runtime/src/binding/property-observation.ts	97.95% <100%> (+2.21%)	⬆️
...ackages/runtime/src/templating/rendering-engine.ts	80.48% <100%> (+0.15%)	⬆️
...kages/runtime/src/templating/view-compile-flags.ts	100% <100%> (ø)
packages/runtime/src/templating/index.ts	100% <100%> (ø)	⬆️
packages/jit/src/templating/template-compiler.ts	91.2% <84.74%> (+2.36%)	⬆️
packages/runtime/src/dom.ts	86.32% <0%> (-1.71%)	⬇️
packages/runtime/src/binding/expression-parser.ts	75% <0%> (-1.48%)	⬇️
... and 6 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc9cbff...f93f3f3. Read the comment docs.

[bigopon]

@fkleuver @EisenbergEffect With the help from Fred, it's now working fine. There are the following issues:

• local dependencies not working
• link-able template controllers not linked
• process content / process attribute
• surrogate instructions

I think it's safe to merge in and then we can continue the above work later.

[fkleuver]

@EisenbergEffect let's get this merged in, I want to get my hands on this stuff :D

[bigopon]

   private templateFromSource(context: IRenderContext, definition: TemplateDefinition): ITemplate {
     if (definition && definition.templateOrNode) {
       if (definition.build.required) {
         const compilerName = definition.build.compiler || defaultCompilerName;
         const compiler = this.compilers[compilerName];
 
         if (!compiler) {
          throw Reporter.error(20, compilerName);
        }
-       definition = compiler.compile(definition, new RuntimeCompilationResources(<ExposedContext>context));
+       definition = compiler.compile(definition, new RuntimeCompilationResources(<ExposedContext>context), ViewCompileFlags.surrogate);
      }

@EisenbergEffect the above changes in rendering-engine may not be correct as I'm not sure how the entire flow is yet.

[bigopon]

@fkleuver Didn't change anything, not sure why circle CI is failing

[EisenbergEffect]

@bigopon I haven't read through all the code yet, but the ViewCompileFlags are new to me. What is the significance of that in this case?

[EisenbergEffect]

@bigopon Looks like there are some merge conflicts now, probably with what I just merged. Sorry! Ping me when that's all fixed up and you want me to do a final review. Thanks for working on this. This is a big milestone!

[fkleuver]

Merge conflict was my bad, it's fixed up now :)
EDIT: some tests are failing, i'm investigating

[fkleuver]

@EisenbergEffect good to merge

[bigopon]

@EisenbergEffect what used to be our ViewCompileInstruction is now ViewCompileFlags. We need a way to tell the compiler when to process surrogate instructions, shadowDOM etc

@fkleuver looks like some changes in my latest commit were lost. Can you add them again. 😁

Edit: Actually we already rebased. I will recommit, it was about surrogate behavior

[bigopon]

@EisenbergEffect @fkleuver It's ready now. Here's how it looks like for normal attribute on a surrogate element:

aurelia/packages/jit/src/templating/template-compiler.ts

Lines 153 to 166 in f93f3f3

	if (name !== 'id' && name !== 'part' && name !== 'replace-part') {
	switch (name) {
	// TODO: handle simple surrogate style attribute
	case 'style':
	attrInst = new SetAttributeInstruction(value, name);
	break;
	default:
	attrInst = new SetAttributeInstruction(value, name);
	break;
	}
	surrogateInstructions.push(attrInst);
	} else {
	throw new Error(`Invalid surrogate attribute: ${name}`);
	}

I'm not sure how to handle style attribute, using SetAttributeInstruction will probably override what the host has before applying. Same for any other attributes. Thoughts ?

[fkleuver]

Hold on, we got a major perf regression for some reason addTodo (x10000) took 2390ms i'm just gonna check what that's all about

[EisenbergEffect]

Some feedback for future work.

[EisenbergEffect]

There's no work to do for slots except that if you find one, you set the hasSlots property of the definition to true.

[EisenbergEffect]

Things like this should use the DOM abstraction.

[EisenbergEffect]

Is this safe? Can't templateOrNode be a string?

[bigopon]

The template of this instruction is from currently processed DOM tree, so it won't be a string

[EisenbergEffect]

Should we improve the createTemplate signature so that it can take the template content as input, optionally?

[bigopon]

@EisenbergEffect Could be nice improvement, it can also take care of adopting node / etc, less code everywhere else.

[EisenbergEffect]

I'm not sure what shadowDOM would do. For vNext, we only need to know if there are slots or not, at least for now.

[fkleuver]

Sorry, go ahead and merge. I'll check the perf later. Might be a circleci hiccup as well