Initial implementation of model inputs #54252

crisbeto · 2024-02-05T11:04:27Z

Adds support for model inputs in the framework. model() returns a writable signal that implicitly defines a input/output pair that can be used either in two-way bindings to keep two values in sync or by binding individually to the input and output. When the value of the model changes, it will emit an event with the current value.

Furthermore, these changes expand two-way bindings to accept WritableSignal. This will make it easier to transition existing code to signals in a backwards-compatible way.

Example:

@Directive({
  selector: 'counter',
  standalone: true,
  host: {
    '(click)': 'increment()',
  }
})
export class Counter {
  value = model(0);

  increment(): void {
    this.value.update(current => current + 1);
  }
}

@Component({
  template: `<counter [(value)]="count"/> The current count is: {{count()}}`,
})
class App {
  count = signal(0);
}

Note: with these changes model inputs should be mostly working. There are a few places where things will be improved in a follow-up PR:

Ensuring that the language service works correctly with the new TCB for two-way bindings.
Avoid having to return an object literal from ModelSignal.subscribe.
Figure out the performance implications of checking if the value is a signal each time the twoWayProperty instruction is called.
Figure out if we should remove applyValueToInputSignal from MODEL_SIGNAL_NODE.
Avoid the extra variable in the two-way binding TCB.
Adding a diagnostic when input() is mixed with @Output() in a two-way binding.

kbrilla · 2024-02-05T13:40:07Z

Questions:

In example we have model(0) and singal(0) so both have same starting value, but which one will win in scenario model(5) and signal(0) ?
Would it be possible to make model required? Or at least input side of model required so there is no need for default value?

crisbeto · 2024-02-05T13:41:46Z

model(0) sets the default value and it'll be overwritten by the one coming from the property binding.

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

packages/core/src/errors.ts

packages/core/src/core.ts

packages/core/src/authoring/model/model_signal_node.ts

devversion

LGTM overall, mostly nits/questions. Nice! 🎉

packages/core/test/authoring/signal_model_signature_test.ts

packages/core/src/authoring/model/model_signal_node.ts

packages/core/src/authoring/model/model_signal.ts

packages/compiler-cli/src/ngtsc/metadata/src/api.ts

packages/compiler-cli/test/initializer_api_transforms_spec.ts

packages/compiler-cli/src/ngtsc/testing/fake_core/index.ts

devversion · 2024-02-06T10:06:38Z

packages/compiler-cli/test/ngtsc/authoring_spec.ts

@@ -199,6 +199,33 @@ runInEachFileSystem(() => {
          .toContain(`inputs: { data: [i0.ɵɵInputFlags.SignalBased, "data"] }`);
    });

+    // TODO(crisbeto): we may not want to support this combination. Will discuss with the team.


Good conversation. I believe we want to support use-cases where the local "state" might be desychronized from the parent. There is a good issue about people asking for this- although in practice we couldn't come up with a good scenario where users may not want to properly synchronize parent & component (e.g. Dir#expanded)

This combination is fine - there's an input and output of the right shape for two-way binding. We can't enforce that components conform to the two-way binding contract.

packages/compiler-cli/test/ngtsc/authoring_spec.ts

devversion · 2024-02-06T10:11:03Z

packages/compiler-cli/src/ngtsc/typecheck/src/template_symbol_builder.ts

+          ts.isAsExpression(node.left.expression.expression) &&
+          isAccessExpression(node.left.expression.expression.expression)) {
+        assignment = node.left.expression.expression.expression;
+      }


Maybe already planned as a follow-up: Can you please add a test for the language service. Definition + completions? I invested a lot in writing tests here and I think we should continue doing this.

Yeah that's one of my points for the follow-up. I just wanted to get all the foundations in place in this PR.

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

Addresses the feedback from angular#54252.

crisbeto · 2024-02-06T13:09:45Z

I've reworked the TCB to avoid some regressions. Also addressed all the feedback in the last commit.

devversion · 2024-02-06T14:00:25Z

packages/compiler-cli/test/ngtsc/authoring_spec.ts

+          standalone: true,
+        })
+        export class TestDir<T> {
+          value = model.required<T>();


We should also have a test to ensure the value property is inferred properly. This is a follow-up I guess?

packages/core/src/render3/instructions/two_way.ts

Addresses the feedback from angular#54252.

crisbeto · 2024-02-09T08:33:05Z

@JeanMeche model() is ready to be used. There are some internal cleanups to do, but they won't affect the behavior.

wartab · 2024-02-09T08:35:32Z

I know things aren't completed yet, but is there anything planned at framework level for forms? Or is the following method the way to go for the foreseeable future?

<input [ngModel]="signal()" (ngModelChange)="signal.set($event)">

sysmat · 2024-02-09T08:38:35Z

@wartab yes as classical prop as signal, for now I'm doing it this way

sysmat · 2024-02-09T08:45:11Z

@JeanMeche so is there another directive for model?

crisbeto · 2024-02-09T09:26:17Z

@sysmat I just tried your example with the RC and it seems to work. See https://stackblitz.com/edit/stackblitz-starters-dcjjnw?file=src%2Fmain.ts

@wartab two-way binding to a signal is supported with this change. You should be able to do [(ngModel)]="someSignal".

wartab · 2024-02-09T09:55:22Z

Really cool, thank you very much

sysmat · 2024-02-09T10:03:30Z

@crisbeto yes it works with signal, but not with model is a bit of mystery to me why not with model, probably I'm missing something here

crisbeto · 2024-02-09T10:06:47Z

It works for me with model() as well. Here's a fork of the previous commit: https://stackblitz.com/edit/stackblitz-starters-kij9th?file=src%2Fmain.ts. Maybe you haven't updated the compiler to the latest RC?

sysmat · 2024-02-09T10:19:02Z

I have

"@angular/compiler-cli": "^17.2.0-rc.0",
"@angular/compiler": "^17.2.0-rc.0",

@crisbeto super, yes I don't now why in my local setup dosen't work

crisbeto · 2024-02-09T10:21:08Z

It may be worth clearing your node_modules and reinstalling.

sysmat · 2024-02-09T10:28:02Z

@crisbeto I delete lock, re install, and also using "builder": "@angular-devkit/build-angular:application",

sysmat · 2024-02-09T10:38:22Z

@crisbeto in your stackblitz you are using, is this ok

"devDependencies": {
    "@angular-devkit/build-angular": "~0.1100.4",
    "@angular/cli": "~11.0.4",
    "@angular/compiler-cli": "~11.0.4",
    "@types/jasmine": "~3.6.0",
    "@types/node": "^12.11.1",
    "codelyzer": "^6.0.0",
    "jasmine-core": "~3.6.0",
    "jasmine-spec-reporter": "~5.0.0",
    "karma": "~5.1.0",
    "karma-chrome-launcher": "~3.1.0",
    "karma-coverage": "~2.0.3",
    "karma-jasmine": "~4.0.0",
    "karma-jasmine-html-reporter": "^1.5.0",
    "protractor": "~7.0.0",
    "ts-node": "~8.3.0",
    "tslint": "~6.1.0",
    "typescript": "~4.0.2"

sysmat · 2024-02-09T10:43:02Z

my component is loaded by router, but this shouldn't be problem, when it works with signal

{
    path: 'model',
    loadComponent: async () => (await import('./app/ng-model.component')).SelectModelSignalComponent,
  }

crisbeto · 2024-02-09T10:43:49Z

Hmm that's weird. Here's a fork with 17.2 of the CLI: https://stackblitz.com/edit/stackblitz-starters-9pipk1?file=package.json

crisbeto · 2024-02-09T11:14:07Z

Following up here: we've released version 17.2.0-rc.1 to resolve the type checking error that @JeanMeche flagged above.

sysmat · 2024-02-09T13:46:54Z

when creating fresh project with cli, it works

Angular CLI: 17.2.0-rc.0
Node: 20.11.0
Package Manager: npm 10.2.4
OS: win32 x64

Angular:
...

Package                      Version
------------------------------------------------------
@angular-devkit/architect    0.1702.0-rc.0 (cli-only)
@angular-devkit/core         17.2.0-rc.0 (cli-only)
@angular-devkit/schematics   17.2.0-rc.0 (cli-only)
@schematics/angular          17.2.0-rc.0 (cli-only)

"dependencies": {
    "@angular/animations": "^17.2.0-next.0",
    "@angular/common": "^17.2.0-next.0",
    "@angular/compiler": "^17.2.0-next.0",
    "@angular/core": "^17.2.0-next.0",
    "@angular/forms": "^17.2.0-next.0",
    "@angular/platform-browser": "^17.2.0-next.0",
    "@angular/platform-browser-dynamic": "^17.2.0-next.0",
    "@angular/router": "^17.2.0-next.0",

VERSION.full --> 17.2.0-rc.1
model & select babnana in the box work

<select [(ngModel)]="selectedModel">
        <option [value]="7">7</option>
        <option [value]="14">14</option>
</select>
<hr />
Selected value: {{ selectedModel() }}

....
version = VERSION.full;
selectedModel = model(7);

so with my old project started as ng 9, doing ng update and migrations, is not working with model but it works with signal

sysmat · 2024-02-09T14:01:56Z

in my new project when using routing then not working model & [()]

export const routes: Routes = [
    {
        path: 'model',
        loadComponent: async () => (await import('./ng-model.component')).SelectModelSignalComponent,
      },
];

export const appConfig: ApplicationConfig = {
  providers: [
    provideRouter(routes, 
    withComponentInputBinding(),
    withViewTransitions(),
    withInMemoryScrolling({
      scrollPositionRestoration: 'enabled',
    }))]
};

sysmat · 2024-02-09T14:21:03Z

I take fork of @crisbeto with routing and it is not working on route (model.ts), but work in main

crisbeto · 2024-02-09T14:27:58Z

I'm guessing this is an issue when setting the value through setInput, which is used by the router.

sysmat · 2024-02-09T14:31:05Z

yes, the withComponentInputBinding() is the problem

scottwalter-nice · 2024-02-09T15:38:05Z

yes, the withComponentInputBinding() is the problem

If I have a component that has no inputs but have withComponentInputBinding() enabled. Declaring a model such as
modelName = model('Model Scott'); will not appear in the template: {{ modelName()}}

Unless I set the value in ngInit: modelName.set('Model Scott');

sysmat · 2024-02-09T17:02:24Z

@scottwalter-nice but why should be model bound to router, for that we have input

scottfwalter · 2024-02-09T18:10:20Z

@scottwalter-nice but why should me model bound to router, for that we have input

I was probably misusing the shiny new model when input or a simple Signal would have sufficed. I guess we need a good explainer on basic SIgnal vs model.

crisbeto · 2024-02-12T07:34:37Z

@sysmat looking into your Stackblitz: I think the problem is that you aren't setting a value for the selectedModel parameter so the router sets it to undefined. E.g. if I set the link to [routerLink]="['/model', {selectedModel: 7}]" it works fine. Here's an updated Stackblitz https://stackblitz.com/edit/stackblitz-starters-gjjmie.

scottwalter-nice · 2024-02-12T12:29:33Z

@sysmat looking into your Stackblitz: I think the problem is that you aren't setting a value for the selectedModel parameter so the router sets it to undefined. E.g. if I set the link to [routerLink]="['/model', {selectedModel: 7}]" it works fine. Here's an updated Stackblitz https://stackblitz.com/edit/stackblitz-starters-gjjmie.

To be honest I’m confused between signal inputs and model. I originally thought model was for two way binding, but i can do that with regular signals on 17.2

What is the use case of model vs signal inputs?

crisbeto · 2024-02-12T12:43:35Z

model() is a shorthand to define a two-way binding.
model signals are writable from inside the component while the input() signals are a read-only.

imaksp · 2024-03-05T05:12:23Z

@scottwalter-nice but why should me model bound to router, for that we have input

I was probably misusing the shiny new model when input or a simple Signal would have sufficed. I guess we need a good explainer on basic SIgnal vs model.

yes I also used new model for ngModel field initially but it didn't work with 'withComponentInputBinding', should have tried this normal signal first. I assume its not meant for using with [(ngModel)].

angular-automatic-lock-bot · 2024-04-05T00:09:08Z

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.}

crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: core Issues related to the framework runtime target: minor This PR is targeted for the next minor release labels Feb 5, 2024

ngbot bot added this to the Backlog milestone Feb 5, 2024

angular-robot bot added the detected: feature PR contains a feature commit label Feb 5, 2024

crisbeto force-pushed the model-inputs branch from fbf7f58 to 8408f50 Compare February 5, 2024 11:16

bart113 mentioned this pull request Feb 5, 2024

[Docs] Signals and forms #50307

Closed

crisbeto force-pushed the model-inputs branch from 8408f50 to cde8507 Compare February 5, 2024 13:33

crisbeto force-pushed the model-inputs branch 3 times, most recently from bfbb7c1 to e17685b Compare February 5, 2024 18:59

crisbeto requested a review from devversion February 5, 2024 19:24

crisbeto marked this pull request as ready for review February 5, 2024 19:24

pullapprove bot requested review from alxhub, dylhunn and pkozlowski-opensource February 5, 2024 19:25

crisbeto commented Feb 5, 2024

View reviewed changes

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts Outdated Show resolved Hide resolved

crisbeto force-pushed the model-inputs branch from e17685b to 1d2606a Compare February 5, 2024 19:31

JeanMeche reviewed Feb 5, 2024

View reviewed changes

packages/core/src/errors.ts Outdated Show resolved Hide resolved

JeanMeche reviewed Feb 5, 2024

View reviewed changes

packages/core/src/core.ts Outdated Show resolved Hide resolved

JeanMeche reviewed Feb 5, 2024

View reviewed changes

packages/core/src/authoring/model/model_signal_node.ts Show resolved Hide resolved

devversion approved these changes Feb 6, 2024

View reviewed changes

crisbeto force-pushed the model-inputs branch from 1d2606a to 74f8d69 Compare February 6, 2024 12:29

crisbeto added a commit to crisbeto/angular that referenced this pull request Feb 6, 2024

refactor(core): address PR feedback

6d4e1b9

Addresses the feedback from angular#54252.

devversion approved these changes Feb 6, 2024

View reviewed changes

devversion reviewed Feb 6, 2024

View reviewed changes

packages/core/src/render3/instructions/two_way.ts Outdated Show resolved Hide resolved

crisbeto added a commit to crisbeto/angular that referenced this pull request Feb 6, 2024

refactor(core): address PR feedback

c5f8911

Addresses the feedback from angular#54252.

FrankVerbeek mentioned this pull request Apr 4, 2024

Setting input signals with Angular component tests not supported cypress-io/cypress#29264

Open

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 5, 2024

Initial implementation of model inputs #54252

Initial implementation of model inputs #54252

Conversation

crisbeto commented Feb 5, 2024 • edited

kbrilla commented Feb 5, 2024 • edited

crisbeto commented Feb 5, 2024

devversion left a comment

Choose a reason for hiding this comment

devversion Feb 6, 2024

Choose a reason for hiding this comment

alxhub Feb 6, 2024

Choose a reason for hiding this comment

devversion Feb 6, 2024

Choose a reason for hiding this comment

crisbeto Feb 6, 2024

Choose a reason for hiding this comment

crisbeto commented Feb 6, 2024

devversion Feb 6, 2024

Choose a reason for hiding this comment

crisbeto commented Feb 9, 2024

wartab commented Feb 9, 2024

sysmat commented Feb 9, 2024

sysmat commented Feb 9, 2024

crisbeto commented Feb 9, 2024

wartab commented Feb 9, 2024

sysmat commented Feb 9, 2024

crisbeto commented Feb 9, 2024

sysmat commented Feb 9, 2024

crisbeto commented Feb 9, 2024

sysmat commented Feb 9, 2024

sysmat commented Feb 9, 2024

sysmat commented Feb 9, 2024

crisbeto commented Feb 9, 2024

crisbeto commented Feb 9, 2024

sysmat commented Feb 9, 2024

sysmat commented Feb 9, 2024

sysmat commented Feb 9, 2024

crisbeto commented Feb 9, 2024

sysmat commented Feb 9, 2024

scottwalter-nice commented Feb 9, 2024

sysmat commented Feb 9, 2024 • edited

scottfwalter commented Feb 9, 2024

crisbeto commented Feb 12, 2024

scottwalter-nice commented Feb 12, 2024

crisbeto commented Feb 12, 2024

imaksp commented Mar 5, 2024

angular-automatic-lock-bot bot commented Apr 5, 2024

crisbeto commented Feb 5, 2024 •

edited

kbrilla commented Feb 5, 2024 •

edited

sysmat commented Feb 9, 2024 •

edited