Repository navigation
Conversation
`ComponentRef.setInput` works by calling `setInputsForProperty` internally. It can be problematic, because it will set the all inputs on the specific node matching the passed-in name, including other directives that aren't associated with the `ComponentRef`. Currently this isn't a problem, because the only way to get a `ComponentRef` is as a result of `ComponentFactory.create` and there is no way to apply other directives through there, however I see a couple of ways that it can manifest itself in the future: 1. Once the runtime for host directives is implemented (after angular#46868), users will be able to apply directives to the root component. 2. If we were to make it possible to inject `ComponentRef` in the future. These changes add some extra logic so that the input is only assigned to the component instance that is associated with the `ComponentRef`.
72ef687 to
5e2a32a
Compare
| if (this._rootLView[index] === this.instance) { | ||
| setInput(lView[TVIEW], this.instance, index, name, dataValue[i + 1] as string, value); | ||
| markDirtyIfOnPush(lView, this._tNode.index); | ||
| break; |
There was a problem hiding this comment.
This break may be problematic if we somehow end up with multiple inputs on the same instance with the same name. Not sure if that's actually possible, but I'm open to removing it.
| reportUnknownPropertyError(message); | ||
| for (let i = 0; i < dataValue.length; i += 2) { | ||
| const index = dataValue[i] as number; | ||
| if (this._rootLView[index] === this.instance) { |
There was a problem hiding this comment.
Figuring out which inputs belong to this instance like this felt a little weird, but the only alternative would be to look at the index and I couldn't find a good way of getting the index of the component instance within the root LView.
| * renderComponent() and ViewContainerRef.createComponent(). | ||
| */ | ||
| export function createRootComponent<T>( | ||
| function createRootComponent<T>( |
There was a problem hiding this comment.
Unrelated change to the rest of the fix, I just saw that this function is only used within the file.
There was a problem hiding this comment.
nit: could we move those refactor changes to a separate (refactor) commit?
| for (let i = 0; i < inputs.length;) { | ||
| const index = inputs[i++] as number; | ||
| const privateName = inputs[i++] as string; | ||
| for (let i = 0; i < inputs.length; i += 2) { |
There was a problem hiding this comment.
This change is also unrelated to the fix, but I think that it's more readable like this compared to incrementing i from within the loop. I'm fine with reverting it as well.
There was a problem hiding this comment.
nit: could we move those refactor changes to a separate (refactor) commit?
crisbeto
added
action: review
pkozlowski-opensource
left a comment
pkozlowski-opensource
left a comment
There was a problem hiding this comment.
@crisbeto there are few things here:
-
the current behaviour of the
setInputassumes that it behaves as a regular binding in a template and this is intentional. IMO we we should define the semantics of the bindings in a template to inputs for components with host directives first and use the same behaviour; in this sense I would rather do the runtime part of host directives first, iron out the semantics and modify the ComponentRef bit then - if needed; -
the assumptions in this PR is that, when binding to a component's inputs, its host directives shouldn't get the binding. This would be different from how the runtime behaves today for directives composed on the same host node. I was under the impression that host directives with exposed inputs should behave as a set of directives on the same node;
-
currently there is no way of writing tests for this change;
I would much rather do the change in the runtime that accounts for the host directives and IMO the setInput should "just work".
So my preference would be to do the setInput change after the regular bindings are figured out.
|
You're right that the intent with host directives is for them to work as if they were applied inside a template. I think that this case is different, because you have a reference pointing to a specific component class. As a user, I would find it odd if calling Small aside regarding tests: I have it on my list to add tests for this specific use case once I start working on the runtime implementation. |
@crisbeto I can see it as a different way of looking at the problem. Again, the initial assumption was that it does behave as a regular input. And it does make total sense to me in case of host directives. We can certainly review this assumption. Maybe an item on the fwk sync agenda? |
|
Sure, let's discuss it. I've added it to the agenda. |
crisbeto
removed
the
action: review
|
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. |
ComponentRef.setInputworks by callingsetInputsForPropertyinternally. It can be problematic, because it will set the all inputs on the specific node matching the passed-in name, including other directives that aren't associated with theComponentRef. Currently this isn't a problem, because the only way to get aComponentRefis as a result ofComponentFactory.createand there is no way to apply other directives through there, however I see a couple of ways that it can manifest itself in the future:ComponentRefin the future.These changes add some extra logic so that the input is only assigned to the component instance that is associated with the
ComponentRef.