Condense element types - remove EmbeddedTemplate and EmbeddedContent #34

mk13 · 2017-04-18T01:08:59Z

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).

…vent decorator found in <template>

…edundant with no real meaningful functionality

ferhatb · 2017-04-18T01:20:04Z

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 ***@***.***> 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: #34 Commit Summary - Added events in EmbeddedTemplateAst and no longer throws error when event decorator found in <template> - 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 - *M* lib/angular_ast.dart <https://github.com/dart-lang/angular_ast/pull/34/files#diff-0> (3) - *M* lib/src/ast.dart <https://github.com/dart-lang/angular_ast/pull/34/files#diff-1> (3) - *D* lib/src/ast/content.dart <https://github.com/dart-lang/angular_ast/pull/34/files#diff-2> (131) - *M* lib/src/ast/element.dart <https://github.com/dart-lang/angular_ast/pull/34/files#diff-3> (20) - *D* lib/src/ast/template.dart <https://github.com/dart-lang/angular_ast/pull/34/files#diff-4> (218) - *M* lib/src/parser/recursive.dart <https://github.com/dart-lang/angular_ast/pull/34/files#diff-5> (259) - *M* lib/src/visitor.dart <https://github.com/dart-lang/angular_ast/pull/34/files#diff-6> (17) - *M* lib/src/visitors/desugar_visitor.dart <https://github.com/dart-lang/angular_ast/pull/34/files#diff-7> (21) - *M* lib/src/visitors/expression_parser_visitor.dart <https://github.com/dart-lang/angular_ast/pull/34/files#diff-8> (17) - *M* lib/src/visitors/humanizing.dart <https://github.com/dart-lang/angular_ast/pull/34/files#diff-9> (45) - *M* lib/src/visitors/identity.dart <https://github.com/dart-lang/angular_ast/pull/34/files#diff-10> (7) - *M* test/parser_test.dart <https://github.com/dart-lang/angular_ast/pull/34/files#diff-11> (66) - *M* test/recover_errors_parser.dart <https://github.com/dart-lang/angular_ast/pull/34/files#diff-12> (93) 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 <#34>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE05qpMtKnTa4IeDClQxmxaKaw2cDkN0ks5rxA0sgaJpZM4M_v5r> .

mk13 · 2017-04-18T17:28:33Z

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

MichaelRFairhurst · 2017-04-18T17:43:22Z

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)

mk13 · 2017-04-18T18:35:52Z

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.

ferhatb · 2017-04-19T14:59:29Z

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

…

On Tue, Apr 18, 2017, 11:35 Max Kim ***@***.***> 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 <#34 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE05qkBdDOJZRXaddgXowFS0zPzt4QTFks5rxQKJgaJpZM4M_v5r> .

MichaelRFairhurst

Apologies for blanking on this for so long. Seems I missed the moment when the original question was answered.

Can't argue its a nice reduction of code!

Did have some structural thoughts, but I think I might leave such structural decisions overall to you and @matanlurey . I can see some value in keeping things a bit more distinct...but definitely to me this looks like a quite nice maintanability change.

MichaelRFairhurst · 2017-05-01T21:57:58Z

lib/src/parser/recursive.dart


-    if (isTemplateElement) {
-      return new EmbeddedTemplateAst.parsed(
+    if (isNgContent) {


What about using a subtyping pattern here?

class NgContentElement extends Element { void reportChildNode(Element childNode, ExceptionHandler handler) { // NgContentElement.reportChildNode throws while Element and TemplateElement accept it } ... }

would remove some of the else-ifiness from the code. Although I suppose it would once again that the instantiation of these would have differences....Hmm, for that, maybe you could make a factory for Element.

factory Element(String tagname, ...) { if (tagname == "template") { return new TemplateElement(tagname, ...); } else ...

I've tried the sub-typing pattern before but it still causes the issue of Element potentially being also either TemplateElement or NgContentElement - and we would just have to check those again.

I haven't tried the factory solution yet, but will try once this issue is picked up again.

MichaelRFairhurst · 2017-05-01T22:00:11Z

lib/src/parser/recursive.dart

        references: references,
        bananas: bananas,
-        stars: stars,
+        stars: (uniqueStarAst == null ? [] : [uniqueStarAst]),


we may want to have all the star asts inside the ElementAst, so that we can, for instance, autocomplete inside them before forcing the user to refactor into two elements with stars.

Sounds like a good idea. I'll probably incorporate this in a separate PR with all the necessary checks needed for this.

mk13 · 2017-05-05T21:14:20Z

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

kevmoo · 2017-10-13T20:51:49Z

Guessing we can close this down, right?

mk13 added 3 commits April 10, 2017 15:42

Added events in EmbeddedTemplateAst and no longer throws error when e…

930f4c0

…vent decorator found in <template>

Merge branch 'master' of https://github.com/dart-lang/angular_ast

0a00490

Removed EmbeddedTemplateAst and EmbeddedContentAst types; they were r…

e5bb810

…edundant with no real meaningful functionality

mk13 requested review from MichaelRFairhurst and matanlurey April 18, 2017 01:08

googlebot added the cla: yes label Apr 18, 2017

Formatting

b5a839b

mk13 mentioned this pull request Apr 18, 2017

Summary integration dart-archive/angular_analyzer_plugin#276

Merged

MichaelRFairhurst approved these changes May 1, 2017

View reviewed changes

matanlurey approved these changes May 5, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Condense element types - remove EmbeddedTemplate and EmbeddedContent #34

Condense element types - remove EmbeddedTemplate and EmbeddedContent #34

Uh oh!

mk13 commented Apr 18, 2017

Uh oh!

ferhatb commented Apr 18, 2017 via email

Uh oh!

mk13 commented Apr 18, 2017

Uh oh!

MichaelRFairhurst commented Apr 18, 2017

Uh oh!

mk13 commented Apr 18, 2017 •

edited

Loading

Uh oh!

ferhatb commented Apr 19, 2017 via email

Uh oh!

MichaelRFairhurst left a comment

Uh oh!

MichaelRFairhurst May 1, 2017

Uh oh!

mk13 May 5, 2017

Uh oh!

MichaelRFairhurst May 1, 2017

Uh oh!

mk13 May 5, 2017

Uh oh!

mk13 commented May 5, 2017

Uh oh!

kevmoo commented Oct 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

Condense element types - remove EmbeddedTemplate and EmbeddedContent #34

Are you sure you want to change the base?

Condense element types - remove EmbeddedTemplate and EmbeddedContent #34

Uh oh!

Conversation

mk13 commented Apr 18, 2017

Uh oh!

ferhatb commented Apr 18, 2017 via email

Uh oh!

mk13 commented Apr 18, 2017

Uh oh!

MichaelRFairhurst commented Apr 18, 2017

Uh oh!

mk13 commented Apr 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ferhatb commented Apr 19, 2017 via email

Uh oh!

MichaelRFairhurst left a comment

Choose a reason for hiding this comment

Uh oh!

MichaelRFairhurst May 1, 2017

Choose a reason for hiding this comment

Uh oh!

mk13 May 5, 2017

Choose a reason for hiding this comment

Uh oh!

MichaelRFairhurst May 1, 2017

Choose a reason for hiding this comment

Uh oh!

mk13 May 5, 2017

Choose a reason for hiding this comment

Uh oh!

mk13 commented May 5, 2017

Uh oh!

kevmoo commented Oct 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

mk13 commented Apr 18, 2017 •

edited

Loading