Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Proposal: expose the AST for expressions parsed through $parse. #16253

Closed
1 of 3 tasks
fpipita opened this issue Oct 4, 2017 · 7 comments
Closed
1 of 3 tasks

Proposal: expose the AST for expressions parsed through $parse. #16253

fpipita opened this issue Oct 4, 2017 · 7 comments

Comments

@fpipita
Copy link
Contributor

fpipita commented Oct 4, 2017

I'm submitting a ...

  • bug report
  • feature request
  • other (Please do not submit support requests here (see above))

Current behavior:

Currently, there seems to be no way to access the AST of expressions parsed through $parse.
Expected / new behavior:

It could be useful to be able to access the AST of parsed expressions in order to avoid building a custom parser to handle advanced use cases in which it is required to work on the expression's AST.

I believe that the format of the current AST is not going to change in the future, so exposing it should not be of concern for possibly breaking changes.

It would be a nice add to have it exposed through the $parse's return value. The new behavior could be made opt-in, just in case keeping the AST "alive" after parsing and for each expression, affects the memory consumption.

Thank you for your time!

AngularJS version: 1.6.6

@petebacondarwin
Copy link
Contributor

This could be achieved by adding the following line before https://github.com/angular/angular.js/blob/master/src/ng/parse.js#L1651

fn.ast = ast;

I am a little worried about exposing this as a "public" API though as it could seriously restrict us from making modification to it in the future.

What if we exposed it as an unsupported hidden feature?? (e.g. fn.$$ast)?

@gkalpak
Copy link
Member

gkalpak commented Oct 4, 2017

Yeah, we definitely don't want to make this public API 😁
The "private" property is fine as long as it doesn't have memory implications (which it probably does 😟).

We could make it configurable via a "private" configuration flag 😁

.config($parseProvider => $parseProvider.$$exposeAst(true))

@fpipita
Copy link
Contributor Author

fpipita commented Oct 4, 2017

Exposing it as an unsupported hidden feature looks good to me!

About the way to configure the opt-in behavior, doing it via the $parseProvider looks ok, but I'd also suggest to give the opportunity to override the global behavior on a per call basis, so something like:

$parse('expression', {$$exposeAst: true})

or:

$parse({expression: 'foo', $$exposeAst: true})

This way we can avoid memory issues by only using this feature in specific use cases that would benefit from it.

Thank you for taking this proposal into account though :)

@petebacondarwin
Copy link
Contributor

Perhaps the second approach?

@fpipita
Copy link
Contributor Author

fpipita commented Oct 5, 2017

I'd pick the second approach too.

Please, let me know if I may submit a PR for this one and I will work on it. Thank you!

@gkalpak
Copy link
Member

gkalpak commented Oct 5, 2017

@fpipita, you are certainly welcome to start working on a PR 😃
We can discuss further details there if necessary. Feel free to ping us if you get stuck.

@fpipita
Copy link
Contributor Author

fpipita commented Oct 6, 2017

Please, let me know the PR needs any further adjustements. Thank you!

gkalpak pushed a commit that referenced this issue Nov 30, 2017
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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants