Repository navigation
feat(ivy): expose node injector as part of debug context - #26210
pkozlowski-opensource wants to merge 1 commit into
Conversation
pkozlowski-opensource
added
action: review
|
You can preview 3532333 at https://pr26210-3532333.ngbuilds.io/. |
3532333 to
4132df3
Compare
|
You can preview 4132df3 at https://pr26210-4132df3.ngbuilds.io/. |
4132df3 to
4378cf7
Compare
|
You can preview 4378cf7 at https://pr26210-4378cf7.ngbuilds.io/. |
4378cf7 to
77a27ae
Compare
|
You can preview 77a27ae at https://pr26210-77a27ae.ngbuilds.io/. |
There was a problem hiding this comment.
Thanks for catching this! Does one of the tests below ensure we won't break it again?
There was a problem hiding this comment.
Yep, the this test would fail without the fix: https://github.com/angular/angular/pull/26210/files#diff-de4113189f21575d1c6ed03b2e4d420bR272
There was a problem hiding this comment.
We already have a getTNode function in instructions.ts, so we probably shouldn't add another one. Seems like we should just add another arg to it for hostView? It should
also probably live here in util, though you'll probably have to move some of the related methods over from context_discovery.
There was a problem hiding this comment.
Decided to drop this util altogether for now as: it is used in one place only and the one in instructions is quite different (LViewData vs. TView, indexes with no offset vs. offset etc.). So the benefit / hassle ratio is too low for me here.
I will probably come back to this if I see this util being beneficial in some other places in the future.
@pkozlowski-opensource Yes please! |
mhevery
left a comment
mhevery
left a comment
There was a problem hiding this comment.
Please take care of the comments, LGTM otherwise. Ping when ready for full approval.
There was a problem hiding this comment.
Just replace getInject implementation above with this implementation.
77a27ae to
1db1433
Compare
|
You can preview 1db1433 at https://pr26210-1db1433.ngbuilds.io/. |
1db1433 to
540c119
Compare
|
You can preview 540c119 at https://pr26210-540c119.ngbuilds.io/. |
540c119 to
7aacfa6
Compare
|
You can preview 7aacfa6 at https://pr26210-7aacfa6.ngbuilds.io/. |
pkozlowski-opensource
added
action: merge
7aacfa6 to
52d2f13
Compare
|
You can preview 52d2f13 at https://pr26210-52d2f13.ngbuilds.io/. |
52d2f13 to
81ba38e
Compare
|
You can preview 81ba38e at https://pr26210-81ba38e.ngbuilds.io/. |
pkozlowski-opensource
added
action: merge
|
Marking as merge-assistance since the google3 presubmit is needed / pending (despite the fact that it was run by Kara: #26210 (comment) |
kara
removed
the
merge: caretaker note
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This PR exposes node injector as part as context discovery utils.
Additionally it fixes a bug where tokens with
NG_ELEMENT_ID=0wouldn't be injected correctly (as0is falsy and(token as any)[NG_ELEMENT_ID] || nullwould always returnnullfor the very first token).@matsko @kara I didn't do this in this PR but I would propose that we:
getInjector()util function;getNodeInjectorproposed here togetInjector()I'm proposing this since I believe that the method proposed here is more useful as end users would probably get access to tokens regardless if those come from a node injector or module injector.