8000 feat(core): add support for two-way bindings on dynamically-created components by crisbeto · Pull Request #60342 · angular/angular · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(core): add support for two-way bindings on dynamically-created components #60342

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.

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

Closed
wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

Builds on the changes from #60137 to add support for two-way bindings on dynamically-created components. Example usage:

import {createComponent, signal, twoWayBinding} from '@angular/core';

const value = signal('');

createComponent(MyCheckbox, {
  bindings: [
    twoWayBinding('value', value),
  ],
});

In the example above the value of MyCheckbox and the value signal will be kept in sync.

@crisbeto crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release labels Mar 12, 2025
@pullapprove pullapprove bot requested review from AndrewKushnir and kirjs March 12, 2025 10:40
@angular-robot angular-robot bot added detected: feature PR contains a feature commit area: core Issues related to the framework runtime labels Mar 12, 2025
@ngbot ngbot bot added this to the Backlog milestone Mar 12, 2025
kind: 'twoWay',
requiredVars: input[BINDING].requiredVars + output[BINDING].requiredVars,
},
set target(target: unknown) {
Copy link
Member Author
@crisbeto crisbeto Mar 12, 2025

Choose a reason for hiding this comment

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

I was debating whether to make this a setter or change target to be a setTarget function. I went with the former, because the only case where it being a function would be useful is two-way bindings.

Copy link
Contributor

Choose a reason for hiding this comment

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

This would work on model inputs too I suppose, even though I didn't see any tests for that, right?

Copy link
Member Author
@crisbeto crisbeto Mar 12, 2025

Choose a reason for hiding this comment

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

Yes it will. I didn't set up any tests for it, because most of our current tests are using JIT and the new function-based APIs need a TS transform to work correctly in JIT.

…omponents

Builds on the changes from angular#60137 to add support for two-way bindings on dynamically-created components. Example usage:

```typescript
import {createComponent, signal, twoWayBinding} from '@angular/core';

const value = signal('');

createComponent(MyCheckbox, {
  bindings: [
    twoWayBinding('value', value),
  ],
});
```

In the example above the value of `MyCheckbox` and the `value` signal will be kept in sync.
@crisbeto crisbeto force-pushed the dynamic-two-way-binding branch from 03a4eb5 to 64658c5 Compare March 12, 2025 12:20
expect(dirInstance.value).toBe('dir changed');
});

it('should not bind root component two-way bindings to directives', () => {

Choose a reason for hiding this comment

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

This was a good test to add 👍

Copy link
Member
@pkozlowski-opensource pkozlowski-opensource left a comment

Choose a reason for hiding this comment

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

LGTM

Reviewed-for: public-api
Reviewed-for: fw-general

@pullapprove pullapprove bot requested a review from alxhub March 17, 2025 10:44
* @param value Writable signal from which to get the current value and to which to write new
* values.
*
* ### Usage example
Copy link
Member

Choose a reason for hiding this comment

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

nit: is it potentially worth to {@link ...} to the API that this function is supposed to be used with?

*
* ### Usage example
* In this example we create an instance of the `MyCheckbox` component and bind to its `value`
* input using a two-way binding.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe not worth doing, but have we thought about some recommendations for type safety here? e.g. potentially users could do for public model fields

twoWayBinding('bla', mySignal as TargetComponent['modelField'])

Copy link
Member
@devversion devversion left a comment

Choose a reason for hiding this comment

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

LGTM for public API; I know per design doc we've decided that we can't reliably support any type safety here

Reviewed-for: public-api

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 17, 2025
@pkozlowski-opensource
Copy link
Member

This PR was merged into the repository by commit b154fb3.

The changes were merged into the following branches: main

@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 Apr 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime detected: feature PR contains a feature commit target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0