Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(@schematics/angular): use signal in app component #29109

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cexbrayat
Copy link
Member

PR Checklist

Please check to confirm 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
  • Other... Please describe:

What is the new behavior?

Now that signals are recommended, this updates the generated AppComponent to use a signal for the title field.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

This updates the generated `AppComponent` to use a signal for the `title` field.
@@ -15,5 +15,5 @@ import { Component } from '@angular/core';
styleUrl: './app.component.<%= style %>'<% } %>
})
export class AppComponent {
title = '<%= name %>';
title = signal('<%= name %>');
Copy link
Collaborator

@alan-agius4 alan-agius4 Dec 11, 2024

Choose a reason for hiding this comment

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

Is it logical for this to be considered a signal, given that it's just a static string? It's feels like this would be a bad usage of signals.

PS: In case we go forward with this we need to update of a couple of other templates.

Copy link
Member Author

Choose a reason for hiding this comment

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

If it really is a static string, then using it directly in the template would make more sense than having a dedicated field no? I always saw it as an example of a dynamic string that the code would update.

Let me know if that looks interesting and I'll update the templates I missed 👍

Copy link
Collaborator

@alan-agius4 alan-agius4 Dec 11, 2024

Choose a reason for hiding this comment

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

I am not too strongly about this, but I wonder if it would send the wrong "signal", that anything referenced in the template needs to be a signal. I'll not directly in the template as it's easier to access in tests.

I'll defer to @alxhub. An alternative for correctness maybe I could see this title having the readonly modifier added.

@alan-agius4 alan-agius4 requested a review from alxhub December 11, 2024 13:51
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants