8000 refactor(core): Ensure that `previousOrParentTNode` always belongs to current `TView`. by mhevery · Pull Request #38707 · angular/angular · GitHub
[go: up one dir, main page]
Skip to content

refactor(core): Ensure that previousOrParentTNode always belongs to current TView. #38707

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
Closed
wants to merge 11 commits into from
Closed

refactor(core): Ensure that previousOrParentTNode always belongs to current TView. #38707

wants to merge 11 commits into from

Conversation

mhevery
Copy link
Contributor
@mhevery mhevery commented Sep 4, 2020 •
edited
Loading

previousOrParentTNode stores current TNode. Due to inconsistent
implementation the value stored would sometimes belong to the current
TView and sometimes to the parent. We have extra logic which accounts
for it. A better solution is to just ensure that previousOrParentTNode
always belongs to current TNode. This simplifies the mental model
and cleans up some code.

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?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@mhevery mhevery force-pushed the 8000 previousOrParentTNodes-always-matches-tview branch from eb1a876 to db86a69 Compare September 4, 2020 05:50
@mhevery mhevery changed the title previousOrParentTNodes always matches tview refactor(core): Ensure that previousOrParentTNode always belongs to current TView. Sep 4, 2020
@mhevery mhevery force-pushed the previousOrParentTNodes-always-matches-tview branch 2 times, most recently from 99845c9 to 790e05e Compare September 4, 2020 19:36
@mhevery mhevery added the target: patch This PR is targeted for the next patch release label Sep 4, 2020
@mhevery mhevery marked this pull request as ready for review September 4, 2020 20:07
@mhevery mhevery force-pushed the previousOrParentTNodes-always-matches-tview branch 3 times, most recently from 27a5be6 to c2663e7 Compare September 4, 2020 21:33
AndrewKushnir
AndrewKushnir reviewed Sep 4, 2020
View reviewed changes
Copy link
Contributor
@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Great refactoring and simplification, thanks Misko! 👍

@pullapprove pullapprove bot requested a review from alxhub September 4, 2020 23:16
@AndrewKushnir AndrewKushnir added area: core Issues related to the framework runtime engine: ivy refactoring Issue that involves refactoring or code-cleanup labels Sep 4, 2020
@ngbot ngbot bot added this to the needsTriage milestone Sep 4, 2020
@AndrewKushnir AndrewKushnir added the action: review The PR is still awaiting reviews from at least one requested reviewer label Sep 4, 2020
petebacondarwin
petebacondarwin reviewed Sep 7, 2020
View reviewed changes
Copy link
Contributor
@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

In general this looks like an improvement to the current situation - I think. That being said I find the instruction state rather confusing since it is difficult to track when it gets updated.

It does seem concerning though, that a non-urgent refactoring brings in two new FIXME comments. There are quite of few of these in the code base now, and I suspect that they are unlikely to attract much attention after this PR lands.
If there is a concern then I feel it would be better to address it earlier rather than later. (I.e. as part of this PR).

mhevery added a commit to mhevery/angular that referenced this pull request Sep 28, 2020
@mhevery
docs(core): correct instructions for running component repo locally (a…
689651b 
…ngular#38707)

PR Close angular#38707
mhevery added a commit to mhevery/angular that referenced this pull request Sep 28, 2020
@mhevery
refactor(core): Ensure that previousOrParentTNode always belongs to…
33aaa9e 
… current `TView`. (angular#38707)

`previousOrParentTNode` stores current `TNode`. Due to inconsistent
implementation the value stored would sometimes belong to the current
`TView` and sometimes to the parent. We have extra logic which accounts
for it. A better solution is to just ensure that `previousOrParentTNode`
always belongs to current `TNode`. This simplifies the mental model
and cleans up some code.

PR Close angular#38707
mhevery added a commit to mhevery/angular that referenced this pull request Sep 28, 2020
@mhevery
refactor(core): Remove host TNode from getOrCreateTNode (angular#…
bc6ff77 
…38707)

Host `TNode` was passed into `getOrCreateTNode` just so that we can
compute weather or not we are a root node. This was needed because
`previousOrParentTNode` could have `TNode` from `TView` other then
current `TView`. This is confusing mental model. Previous change
ensured that `previousOrParentTNode` must always be part of `TView`,
which enabled this change to remove the unneeded argument.

PR Close angular#38707
mhevery added a commit to mhevery/angular that referenced this pull request Sep 28, 2020
@mhevery
refactor(core): Add injector debug information to LViewDebug (angul…
b613639 
…ar#38707)

Extended the `LViewDebug` to display node-injector information for each
node.

PR Close angular#38707
mhevery added a commit to mhevery/angular that referenced this pull request Sep 28, 2020
@mhevery
refactor(core): Remove reliance on TNodeType.View. (angular#38707)
4645f43 
`TNodeType.View` was created to support inline views. That feature did
not materialize and we have since removed the instructions for it, leave
 an unneeded `TNodeType.View` which was still used in a very
 inconsistent way. This change no longer created `TNodeType.View` (and
 there will be a follow up chang to completely remove it.)

Also simplified the mental model so that `LView[HOST]`/`LView[T_HOST]`
always point to the insertion location of the `LView`.

PR Close angular#38707
mhevery added a commit to mhevery/angular that referenced this pull request Sep 28, 2020
@mhevery
refactor(core): Remove TViewNode as it is no longer used. (angular#…
ed35adb 
…38707)

Previous commit change the logic to not rely on the `TViewNode` this
change removes it entirely.

PR Close angular#38707
mhevery added a commit to mhevery/angular that referenced this pull request Sep 28, 2020
@mhevery
refactor(core): Rename TView.node to TView.declTNode. (angular#38707
15fa4bb 
)

The value stored in `TView.node` is really the declaration `TNode`,
therefore renaming to make it more explicit.

PR Close angular#38707
mhevery added a commit to mhevery/angular that referenced this pull request Sep 28, 2020
@mhevery
refactor(core): change getPreviousOrParentTNode to return `TNode|nu…
0fa208f 
…ll` (angular#38707)

This change makes `getPreviousOrParentTNode` return `TNode|null` (rather
than just `TNode`) which is more reflective of the reality. The
`getPreviousOrParentTNode` can be `null` upon entering the `LView`.

PR Close angular#38707
mhevery added a commit to mhevery/angular that referenced this pull request Sep 28, 2020
@mhevery
refactor(core): renamed previousOrParent to currentTNode (angular…
65d4e7a 
…#38707)

The previous name of `previousOrParent` was confusing. Changed the
terminology to `currentTNode`.

PR Close angular#38707
mhevery added a commit to mhevery/angular that referenced this pull request Sep 28, 2020
@mhevery
docs(core): Update instructions on updating symbol tests (angular#38707)
9891cef 
PR Close angular#38707
mhevery added a commit to mhevery/angular that referenced this pull request Sep 28, 2020
@mhevery
refactor(core): Create NodeInjectorOffset type which better describ…
69302ad 
…es NodeInjector (angular#38707)

`NodeInjector` is store in expando as a list of values in an array. The
offset constant into the array have been brought together into a single
`NodeInjectorOffset` enum with better documentation explaining their usage.

PR Close angular#38707
@mhevery mhevery mentioned this pull request Sep 28, 2020
Merged
14 tasks
@AndrewKushnir AndrewKushnir mentioned this pull request Oct 15, 2020
Open
@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 Oct 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@petebacondarwin petebacondarwin petebacondarwin approved these changes

@alfaproject alfaproject alfaproject left review comments

@jelbourn jelbourn jelbourn left review comments

@IgorMinar IgorMinar IgorMinar approved these changes

@gkalpak gkalpak gkalpak approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime cla: yes refactoring Issue that involves refactoring or code-cleanup target: major This PR is targeted for the next major release
Projects
None yet
Milestone
needsTriage
Development

Successfully merging this pull request may close these issues.
9 participants
@mhevery @AndrewKushnir @petebacondarwin @alxhub @alfaproject @IgorMinar @jelbourn @gkalpak @googlebot
0