angular_ast icon indicating copy to clipboard operation
angular_ast copied to clipboard

Published 20 hours ago • verified publisher icon dart-archive

Condense element types - remove EmbeddedTemplate and EmbeddedContent

Open mk13 opened this issue 8 years ago • 7 comments
trafficstars

This is a pretty big design change - I've decided to essentially remove 'EmbeddedTemplateAst' and 'EmbeddedContentAst'.

TL;DR: Having three types had no purpose and just overcomplicated things.

Reasoning:

  • At a class level, these two weren't doing anything unique. They were both just essentially a 'gutted' version of 'ElementAst' with different class names. They're all just containers to begin with.
  • Instead of using the class name to differentiate, just use a simple bool flag within the ElementAst (isTemplate and isNgContent).
  • Building from above, this allows for a 'common type' where users won't have to always check if the type is ElementAst, EmbeddedContentAst, or EmbeddedTemplateAst. All the appropriate irrelevant fields are already emptied out (Example: if name is 'ng-content', bananas, events, properties, etc. are all empty lists).

mk13 avatar Apr 18 '17 01:04 mk13

Have you looked at AngularDart source to see if change in visitors will be ok?

A lot of this mirrors existing code to make migrating maybe easier?

On Mon, Apr 17, 2017, 18:09 Max Kim [email protected] wrote:

This is a pretty big design change - I've decided to essentially remove 'EmbeddedTemplateAst' and 'EmbeddedContentAst'.

TL;DR: Having three types had no purpose and just overcomplicated things.

Reasoning:

  • At a class level, these two weren't doing anything unique. They were both just essentially a 'gutted' version of 'ElementAst' with different class names. They're all just containers to begin with.
  • Instead of using the class name to differentiate, just use a simple bool flag within the ElementAst (isTemplate and isNgContent).
  • Building from above, this allows for a 'common type' where users won't have to always check if the type is ElementAst, EmbeddedContentAst, or EmbeddedTemplateAst. All the appropriate irrelevant fields are already emptied out (Example: if name is 'ng-content', bananas, events, properties, etc. are all empty lists).

You can view, comment on, or merge this pull request online at:

https://github.com/dart-lang/angular_ast/pull/34 Commit Summary

  • Added events in EmbeddedTemplateAst and no longer throws error when event decorator found in
  • Merge branch 'master' of https://github.com/dart-lang/angular_ast
  • Removed EmbeddedTemplateAst and EmbeddedContentAst types; they were redundant with no real meaningful functionality

File Changes

Patch Links:

  • https://github.com/dart-lang/angular_ast/pull/34.patch
  • https://github.com/dart-lang/angular_ast/pull/34.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/dart-lang/angular_ast/pull/34, or mute the thread https://github.com/notifications/unsubscribe-auth/AE05qpMtKnTa4IeDClQxmxaKaw2cDkN0ks5rxA0sgaJpZM4M_v5r .

ferhatb avatar Apr 18 '17 01:04 ferhatb

I'll take a look at the AngularDart code to see how they distinguish these types and see how their visitors behave.

mk13 avatar Apr 18 '17 17:04 mk13

Its also possible to create a base/mixin class which distinguishes these, ie,

abstract class ElementTypeDistinguishingVisitorMixin implements AngularAstVisitor {
  void visitElementAst(ElementAst ast) {
    if (ast.name == 'template') {
      visitEmbeddedTemplateAst(ast);
    } else if (ast.name == 'ng-content') {
      visitEmbeddedContentAst(ast);
    } else {
      visitNormalElementAst(ast);
    }
  }

  void visitEmbeddedTemplateAst(ElementAst ast);
  void visitEmbeddedContentAst(ElementAst ast);
  void visitNormalElementAst(ElementAst ast);
}

class PreexistingAngularVisitor with ElementTypeDistinguishingVisitorMixin {
   // hopefully relatively few changes here
}

Alternatively, all three classes could share an interface and we could use

abstract class HandleEmbeddedContentLikeNormalVisitorMixin implements TemplateAstVisitor {
    void visitCommonElement(CommonElementAst ast);

    void visitElementAst(ElementAst ast) {
      visitCommonElement(ast);
    }

    void visitEmbeddedContentAst(EmbeddedContentAst ast) {
      visitCommonElement(ast);
    }

    void visitEmbeddedTemplateAst(EmbeddedTemplateAst ast) {
      visitCommonElement(ast);
    }
}

and alternatively alternatively, we can use composition over inheritance if we want to prevent accidental overriding of the special mixin methods. (the "normalizing" visitor is a regular visitor, but accepts a "normalized" visitor which has no method visitElementAst, only a method visitCommonElement, and the normalizing visitor just forwards the visitations)

MichaelRFairhurst avatar Apr 18 '17 17:04 MichaelRFairhurst

I looked through the visitors in AngularDart and it seems that the only visitor that does things differently for the three element types is only in ViewBuilderVisitor.

The others (ViewBinderVisitor, TemplateHumanizer, TemplateContentProjectionHumanizer) have a lot of overlapping behavior within all three types (ElementAst, NgContent, EmbeddedTemplateAst) and can realistically be merged into a single visitor (visitElementAst) that still maintains the intended behavior.

If there are no plans to expand the number of visitors, then the Mixin might even be overkill since it will only realistically be applied to ViewBuilderVisitor.

mk13 avatar Apr 18 '17 18:04 mk13

SG. If thats the only visitor, please go ahead with unifying.

On Tue, Apr 18, 2017, 11:35 Max Kim [email protected] wrote:

I looked through the visitors in AngularDart and it seems that the only visitor that does things differently for the three is only in ViewBuilderVisitor.

The others (ViewBinderVisitor, TemplateHumanizer, TemplateContentProjectionHumanizer) have a lot of overlapping behavior within all three types (ElementAst, NgContent, EmbeddedTemplateAst).

If there are no plans to expand the number of visitors, then the Mixin might even be overkill since it will only realistically be applied to ViewBuilderVisitor.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/dart-lang/angular_ast/pull/34#issuecomment-294940164, or mute the thread https://github.com/notifications/unsubscribe-auth/AE05qkBdDOJZRXaddgXowFS0zPzt4QTFks5rxQKJgaJpZM4M_v5r .

ferhatb avatar Apr 19 '17 14:04 ferhatb

Putting this PR onto a freeze until more PoC can be done on how well it can integrate into angular2.

mk13 avatar May 05 '17 21:05 mk13

Guessing we can close this down, right?

kevmoo avatar Oct 13 '17 20:10 kevmoo