Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(compiler): give ASTWithSource its own visit method #31347

Closed
wants to merge 2 commits into from
Closed

fix(compiler): give ASTWithSource its own visit method #31347

wants to merge 2 commits into from

Conversation

ayazhafiz
Copy link
Member

ASTWithSource contains more information that AST and should have its own
visit method, if desired. This implements that.

Part 1 of template parsing refactoring for stability.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

ASTWithSource#visit defers to AST#visit.

Issue Number: N/A

What is the new behavior?

ASTWithSource#visit has its own method if applicable, otherwise defers to AST#visit.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@ayazhafiz ayazhafiz added type: bug/fix target: major This PR is targeted for the next major release area: compiler Issues related to `ngc`, Angular's template compiler labels Jun 28, 2019
@ayazhafiz ayazhafiz requested a review from a team as a code owner June 28, 2019 23:14
@ayazhafiz ayazhafiz self-assigned this Jun 28, 2019
@ngbot ngbot bot modified the milestone: needsTriage Jun 28, 2019
@ayazhafiz ayazhafiz requested a review from alxhub June 28, 2019 23:14
alxhub
alxhub requested changes Jul 1, 2019
View reviewed changes
Copy link
Member

@alxhub alxhub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks okay, but the diff is bigger than expected. Rebase needed?

@ayazhafiz
fix(compiler): give ASTWithSource its own visit method
7fbe691 
ASTWithSource contains more information that AST and should have its own
visit method, if desired. This implements that.
@ayazhafiz
fixup! fix(compiler): give ASTWithSource its own visit method
fcf6d54
@ayazhafiz ayazhafiz force-pushed the feature/ast-source-own-visitor branch from 35a992e to fcf6d54 Compare July 2, 2019 00:19
@ayazhafiz
Copy link
Member Author

This looks okay, but the diff is bigger than expected. Rebase needed?

No; the diff is large because for whatever reason template_spec didn't receive the runInEachFileSystem wrapper that was introduced in 7186f9c. The tests fail without adding this.

@ayazhafiz ayazhafiz requested a review from alxhub July 2, 2019 00:21
@ayazhafiz ayazhafiz mentioned this pull request Jul 2, 2019
Closed
14 tasks
alxhub
alxhub approved these changes Jul 3, 2019
View reviewed changes
Copy link
Member

@alxhub alxhub left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ayazhafiz ayazhafiz added the action: merge The PR is ready for merge by the caretaker label Jul 3, 2019
@jasonaden jasonaden closed this in 6aaca21 Jul 8, 2019
@ayazhafiz ayazhafiz deleted the feature/ast-source-own-visitor branch July 8, 2019 17:39
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alxhub alxhub alxhub approved these changes

Assignees

@ayazhafiz ayazhafiz

Labels
action: merge The PR is ready for merge by the caretaker area: compiler Issues related to `ngc`, Angular's template compiler cla: yes target: major This PR is targeted for the next major release type: bug/fix
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ayazhafiz @alxhub @googlebot