feat($parse): enable optional access to the underlying AST #16260

fpipita · 2017-10-06T08:39:21Z

This PR adds a new private method to the $parse service, $$getAst,
which takes an Angular expression as its only argument and returns
the computed AST. This feature is not meant to be part of the public
API and might be subject to changes, so use it with caution.

Closes #16253

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Feature.

What is the current behavior? (You can also link to an open issue here)
#16253

What is the new behavior (if this is a feature change)?
The new opt-in behavior introduces a way to expose the ast.

Does this PR introduce a breaking change?
No.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

petebacondarwin

Might be worth mentioning the potential memory issues with this in the documentation.

gkalpak

Looking good overall 👍 I left a couple of (mostly minor) comments.

gkalpak · 2017-10-06T09:19:32Z

src/ng/parse.js

@@ -1763,6 +1768,24 @@ function $ParseProvider() {
    return this;
  };

+    /**
+   * @ngdoc method


Since this is supposed to be private API, we don't want it to appear in the public docs.

gkalpak · 2017-10-06T09:20:51Z

src/ng/parse.js

+   * This feature is not meant to be part of the public API, so use it at your own risk.
+   *
+   * @param {boolean} exposeAst if set to true, the AST of each parsed expression will be exposed through
+   * an `$$ast` property added to the $parse's return value.


the $parse's return value --> `$parse`'s return value

gkalpak · 2017-10-06T09:23:37Z

src/ng/parse.js

+    if (isBoolean(exposeAst)) {
+      globalExposeAst = exposeAst;
+    }
+  };


It might be better to stick to the getter/setter model we use in other places:

this.$$exposeAst = function(exposeAst) { if (isUndefined(exposeAst)) { // Getter return globalExposeAst; } else { // Setter globalExposeAst = !!exposeAst; // or: (exposeAst === true); return this; } }

gkalpak · 2017-10-06T09:32:52Z

src/ng/parse.js

@@ -1790,7 +1821,7 @@ function $ParseProvider() {
            }
            var lexer = new Lexer($parseOptions);
            var parser = new Parser(lexer, $filter, $parseOptions);
-            parsedExpression = parser.parse(exp);
+            parsedExpression = parser.parse(exp, finalExposeAst);
            parsedExpression.oneTime = !!oneTime;

            cache[cacheKey] = addWatchDelegate(parsedExpression);


One problem is that cached parsedExpressions will always follow the $$exposeAst from the first call. E.g.:

$parse('user.name'); // --> No `$$ast` (expected) $parse({expression: 'user.name', $$exposeAst: true}); // --> No `$$ast` (ooops)

Not sure what is the best way to handle that (without impacting uninterested users) 😕

Also, theoretically, the addInterceptor() function should preserve the $$ast property, but we can assume nobody using $$exposeAst cares about interceptors at this point. Maybe at least add a comment somewhere (that adding an interceptor won't preserve the exposed $$ast (if any)).

gkalpak · 2017-10-06T09:33:49Z

src/ng/parse.js

@@ -1775,6 +1798,14 @@ function $ParseProvider() {

    function $parse(exp, interceptorFn) {
      var parsedExpression, oneTime, cacheKey;
+      var finalExposeAst = globalExposeAst;


The final prefix is kind of confusing imo. I would just call it exposeAst (or localExposeAst).

gkalpak · 2017-10-06T09:40:18Z

test/ng/parseSpec.js

+      it('should be forbidden by default', inject(function($parse) {
+        var fn = $parse('foo.bar');
+        expect(fn.$$ast).toBeUndefined();
+      }));


Can you also add a test with $parse({expression: '...'})?

gkalpak · 2017-10-06T09:40:40Z

test/ng/parseSpec.js

+      it('should be configurable on a per call basis', inject(function($parse) {
+        var fn = $parse({expression: 'foo.bar', $$exposeAst: true});
+        expect(fn.$$ast).toBeDefined();
+      }));


Can you also add a test with $$exposeAst: false?

gkalpak · 2017-10-06T09:47:03Z

src/ng/parse.js

@@ -1763,6 +1768,24 @@ function $ParseProvider() {
    return this;
  };

+    /**


Indentation isssue 😱 😱 😱 😱 😱 😱 😱

petebacondarwin · 2017-10-06T09:53:14Z

Thinking more about this...

Would it be simpler just to expose a method $parse.$$getAst(expression) instead? The lexer and ast builder will already be set up; and this would avoid all of these extra provider flags and mucking about with the parameters of a public API service.

fpipita · 2017-10-06T10:31:34Z

@petebacondarwin your proposal looks actually cleaner. I'll rework the PR and follow the new approach.

fpipita · 2017-10-07T16:46:58Z

Hi, I've reworked the PR and implemented the new behavior.

I didn't fixup the commits yet to prevent the comments by @gkalpak from being removed.

petebacondarwin

This is much better and cleaner from an API point of view. I am frustrated by the code duplication between the new $$getAst function and the $parse function.

I think that we can get the code size and complexity down a bit by refactoring the Parser to expose a new function getAst(). The cost of creating a parser is pretty small (it has to create an ASTCompiler/ASTInterpreter but their constructors are very simple).

Moreover we could move the parsing of the one-time binding flag (::) into a helper in the Parser which might save some more lines.

But these are internal refactorings that I would be happy to land in subsequent PRs.

gkalpak · 2017-10-09T20:15:29Z

src/ng/parse.js

+    }
+
+    function isOneTimeBinding(exp) {
+      if (typeof exp !== 'string') {


We don't need this check afaict.

I've added this check to future-proof this utility function, just in case we wanted to use it in contexts where it is not safe to assume that the caller will pass it a string.

If we can assume that it'll always be called with a string then I'll remove the check.

gkalpak · 2017-10-09T20:36:05Z

I just realized I have no idea how you plan to use the returned AST. Can you give an example of how you would use this feature?

petebacondarwin · 2017-10-09T22:11:49Z

@gkalpak see #16253 (comment)

gkalpak · 2017-10-10T05:41:17Z

Yes, I've seen that. I am just curious what are they going to do with the AST?

fpipita · 2017-10-10T08:00:42Z

@petebacondarwin thank you for your feedback, I can further improve the PR with the refactorings. That duplication made me quite frustrated as well!

@gkalpak sure, I'll share what I'm doing :)

I've built a form field component which can be used like:

<cm-form-field>
  <!-- it can be anything bound to a model through ng-model -->
  <input ng-model="foo.bar.description" />
</cm-form-field>

The component has three main responsibilities:

It abstracts away the underlaying ui framework (I'm using Bootstrap) by taking care of generating the required markup;
It takes care of adding a default label for the input field by using the convention that the label is named the same way as the _.startCase'd name of identifier ng-model is bound to, so in the above example, the input field will get a default label of "Description";
It also adds the name attribute to the input field which by convention is exactly the same as the identifier's one;

It works in collaboration with an overloaded version of ng-model, which does the job of informing the cmFormField's controller about the identifier's name.

To extract the identifier's name, ng-model needs to parse the expression it is bound to and, at the moment, I'm achieving it by using a regular expression. It works fairly well for simple use cases, but if I wanted to do anything more complex (like detecting a computed property's name), I fear things can get quickly messy :)

<cm-form-field>
  <input ng-model="foo[$ctrl.bar]" />
</cm-form-field>

By using the AST, it would be easy to do it, instead.

gkalpak · 2017-10-10T08:29:28Z

@fpipita, I see. Pretty interesting usecase. Thx for sharing 😃

fpipita · 2017-11-19T12:44:50Z

@petebacondarwin I've reworked a bit the PR in order to remove the code duplication and I've added the changes as a distinct commit so it is easier to inspect them and eventually discard them.

I've also removed the two initial commits related to the first implementation, since everyone agreed about it looking quite bad.

I'm almost entirely happy with how it looks right now, except for the only bit I wasn't able to deduplicate, the exp.trim() call, which is still done twice, in both $parse and in the newly added Parser.prototype.getAst.

gkalpak

LGTM (although this PR description needs updating).
I wonder if we want to show this commit on the changelog (considering it is a "non-public" feature).

fpipita · 2017-11-27T08:27:30Z

@gkalpak I've updated the PR description, thank you for pointing that out.

Do you think that the refactor tag would be appropriate for the type of changes? If not, which one would you suggest to use?

One last thing .. will you please let me know whether I can merge the two commits? :)

petebacondarwin · 2017-11-27T10:53:36Z

Please do squash the two commits.

I think it is OK to include it as a feat commit. As long as the description makes it clear that it is a private feature and that people should not rely on it being stable. (There is little point in hiding it, since it is simple to just read through the commits if one really wants).

This PR adds a new private method to the `$parse` service, `$$getAst`, which takes an Angular expression as its only argument and returns the computed AST. This feature is not meant to be part of the public API and might be subject to changes, so use it with caution. Closes #16253

fpipita · 2017-11-28T16:00:22Z

Hi, I've squashed the two commits and tried to make it clearer through the description that this is a private feature.

gkalpak · 2017-11-28T17:11:04Z

Still LGTM (restarted Travis - the failures looked like flakes)

This PR adds a new private method to the `$parse` service, `$$getAst`, which takes an Angular expression as its only argument and returns the computed AST. This feature is not meant to be part of the public API and might be subject to changes, so use it with caution. Closes #16253 Closes #16260

googlebot added the cla: yes label Oct 6, 2017

petebacondarwin approved these changes Oct 6, 2017

View reviewed changes

gkalpak suggested changes Oct 6, 2017

View reviewed changes

gkalpak reviewed Oct 6, 2017

View reviewed changes

Narretz added this to the Backlog milestone Oct 9, 2017

Narretz added component: $parse type: feature labels Oct 9, 2017

petebacondarwin approved these changes Oct 9, 2017

View reviewed changes

gkalpak reviewed Oct 9, 2017

View reviewed changes

gkalpak approved these changes Nov 21, 2017

View reviewed changes

petebacondarwin added the needs: merging label Nov 29, 2017

gkalpak closed this in 2e03aed Nov 30, 2017

dependencies bot mentioned this pull request Dec 22, 2017

Update 4 dependencies from npm JetBrains/ring-ui#49

Merged

fpipita deleted the ast branch May 17, 2018 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat($parse): enable optional access to the underlying AST #16260

feat($parse): enable optional access to the underlying AST #16260

fpipita commented Oct 6, 2017 •

edited

Loading

petebacondarwin left a comment

gkalpak left a comment

gkalpak Oct 6, 2017

gkalpak Oct 6, 2017

gkalpak Oct 6, 2017

gkalpak Oct 6, 2017

gkalpak Oct 6, 2017

gkalpak Oct 6, 2017

gkalpak Oct 6, 2017

gkalpak Oct 6, 2017

petebacondarwin commented Oct 6, 2017

fpipita commented Oct 6, 2017

fpipita commented Oct 7, 2017

petebacondarwin left a comment

gkalpak Oct 9, 2017

fpipita Oct 10, 2017

gkalpak commented Oct 9, 2017

petebacondarwin commented Oct 9, 2017

gkalpak commented Oct 10, 2017

fpipita commented Oct 10, 2017 •

edited

Loading

gkalpak commented Oct 10, 2017

fpipita commented Nov 19, 2017

gkalpak left a comment

fpipita commented Nov 27, 2017

petebacondarwin commented Nov 27, 2017

fpipita commented Nov 28, 2017

gkalpak commented Nov 28, 2017

feat($parse): enable optional access to the underlying AST #16260

feat($parse): enable optional access to the underlying AST #16260

Conversation

fpipita commented Oct 6, 2017 • edited Loading

petebacondarwin left a comment

Choose a reason for hiding this comment

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Oct 6, 2017

fpipita commented Oct 6, 2017

fpipita commented Oct 7, 2017

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak commented Oct 9, 2017

petebacondarwin commented Oct 9, 2017

gkalpak commented Oct 10, 2017

fpipita commented Oct 10, 2017 • edited Loading

gkalpak commented Oct 10, 2017

fpipita commented Nov 19, 2017

gkalpak left a comment

Choose a reason for hiding this comment

fpipita commented Nov 27, 2017

petebacondarwin commented Nov 27, 2017

fpipita commented Nov 28, 2017

gkalpak commented Nov 28, 2017

fpipita commented Oct 6, 2017 •

edited

Loading

fpipita commented Oct 10, 2017 •

edited

Loading