Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(core): add toString implementation to signals #54002

Closed
wants to merge 1 commit into from
Closed

fix(core): add toString implementation to signals #54002

wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Jan 20, 2024 •
edited

Since signals are functions, currently stringifying them reveals the implementation of the function. This can lead to confusion since it contains internal implementation details. These changes add static toString function to address the issue.

Note: it's tempting to have toString output the actual value of the signal, but that would encourage users not to call the function which will be problematic in the long run. That's why these changes are using a static string instead.

@crisbeto 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: patch This PR is targeted for the next patch release labels Jan 20, 2024
@ngbot ngbot bot added this to the Backlog milestone Jan 20, 2024
@crisbeto crisbeto marked this pull request as ready for review January 20, 2024 08:53
@eneajaho
Copy link
Contributor

Isn't this method a great use for the dev tools feature where we can see the Signals graph?

@crisbeto
Copy link
Member Author

I'm not sure if it's worth shipping the extra code for traversing the graph, given that the same information can be displayed better in the dev tools.

pkozlowski-opensource
pkozlowski-opensource approved these changes Jan 25, 2024
View reviewed changes
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. But now we've got also query signals so you might want to include those as well - or I can do it in the follow-up PR.

@crisbeto
fix(core): add toString implementation to signals
1eeee66 
Since signals are function, currently stringifying them reveals the implementation of the function. This can lead to confusion since it contains internal implementation details. These changes add static `toString` function to address the issue.

**Note:** it's tempting to have `toString` output the actual value of the signal, but that would encourage users not to call the function which will be problematic in the long run. That's why these changes are using a static string instead.
@crisbeto
Copy link
Member Author

Updated with a toString implementation for query signals.

@crisbeto crisbeto added action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release and removed action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release labels Jan 25, 2024
@jessicajaniuk
Copy link
Contributor

This PR was merged into the repository by commit 656bc28.

crisbeto added a commit to crisbeto/angular that referenced this pull request Jan 25, 2024
@crisbeto
refactor(core): signals toString improvements
d82f3eb 
Follow-up to angular#54002 that:
* Remove the `toString` implementation from the `primitives`.
* Guards the `toString` with `ngDevMode` and prints out the value.
@crisbeto crisbeto mentioned this pull request Jan 25, 2024
Closed
jessicajaniuk pushed a commit that referenced this pull request Jan 25, 2024
@crisbeto @thePunderWoman
refactor(core): signals toString improvements (#54079)
c637dfa 
Follow-up to #54002 that:
* Remove the `toString` implementation from the `primitives`.
* Guards the `toString` with `ngDevMode` and prints out the value.

PR Close #54079
atscott pushed a commit to atscott/angular that referenced this pull request Jan 26, 2024
@crisbeto @atscott
fix(core): add toString implementation to signals (angular#54002)
3511645 
Since signals are function, currently stringifying them reveals the implementation of the function. This can lead to confusion since it contains internal implementation details. These changes add static `toString` function to address the issue.

**Note:** it's tempting to have `toString` output the actual value of the signal, but that would encourage users not to call the function which will be problematic in the long run. That's why these changes are using a static string instead.

PR Close angular#54002
rlmestre pushed a commit to rlmestre/angular that referenced this pull request Jan 26, 2024
@crisbeto @rlmestre
fix(core): add toString implementation to signals (angular#54002)
b137f09 
Since signals are function, currently stringifying them reveals the implementation of the function. This can lead to confusion since it contains internal implementation details. These changes add static `toString` function to address the issue.

**Note:** it's tempting to have `toString` output the actual value of the signal, but that would encourage users not to call the function which will be problematic in the long run. That's why these changes are using a static string instead.

PR Close angular#54002
rlmestre pushed a commit to rlmestre/angular that referenced this pull request Jan 26, 2024
@crisbeto @rlmestre
refactor(core): signals toString improvements (angular#54079)
7a07993 
Follow-up to angular#54002 that:
* Remove the `toString` implementation from the `primitives`.
* Guards the `toString` with `ngDevMode` and prints out the value.

PR Close angular#54079
danieljancar pushed a commit to danieljancar/angular that referenced this pull request Jan 26, 2024
@crisbeto @danieljancar
fix(core): add toString implementation to signals (angular#54002)
49df4a7 
Since signals are function, currently stringifying them reveals the implementation of the function. This can lead to confusion since it contains internal implementation details. These changes add static `toString` function to address the issue.

**Note:** it's tempting to have `toString` output the actual value of the signal, but that would encourage users not to call the function which will be problematic in the long run. That's why these changes are using a static string instead.

PR Close angular#54002
danieljancar pushed a commit to danieljancar/angular that referenced this pull request Jan 26, 2024
@crisbeto @danieljancar
refactor(core): signals toString improvements (angular#54079)
b6093db 
Follow-up to angular#54002 that:
* Remove the `toString` implementation from the `primitives`.
* Guards the `toString` with `ngDevMode` and prints out the value.

PR Close angular#54079
amilamen pushed a commit to amilamen/angular that referenced this pull request Jan 26, 2024
@crisbeto @amilamen
fix(core): add toString implementation to signals (angular#54002)
b6ac118 
Since signals are function, currently stringifying them reveals the implementation of the function. This can lead to confusion since it contains internal implementation details. These changes add static `toString` function to address the issue.

**Note:** it's tempting to have `toString` output the actual value of the signal, but that would encourage users not to call the function which will be problematic in the long run. That's why these changes are using a static string instead.

PR Close angular#54002
amilamen pushed a commit to amilamen/angular that referenced this pull request Jan 26, 2024
@crisbeto @amilamen
refactor(core): signals toString improvements (angular#54079)
283f3c8 
Follow-up to angular#54002 that:
* Remove the `toString` implementation from the `primitives`.
* Guards the `toString` with `ngDevMode` and prints out the value.

PR Close angular#54079
nikvarma pushed a commit to nikvarma/angular that referenced this pull request Jan 31, 2024
@crisbeto @nikvarma
fix(core): add toString implementation to signals (angular#54002)
d845551 
Since signals are function, currently stringifying them reveals the implementation of the function. This can lead to confusion since it contains internal implementation details. These changes add static `toString` function to address the issue.

**Note:** it's tempting to have `toString` output the actual value of the signal, but that would encourage users not to call the function which will be problematic in the long run. That's why these changes are using a static string instead.

PR Close angular#54002
nikvarma pushed a commit to nikvarma/angular that referenced this pull request Jan 31, 2024
@crisbeto @nikvarma
refactor(core): signals toString improvements (angular#54079)
dacb037 
Follow-up to angular#54002 that:
* Remove the `toString` implementation from the `primitives`.
* Guards the `toString` with `ngDevMode` and prints out the value.

PR Close angular#54079
@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 Feb 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pkozlowski-opensource pkozlowski-opensource pkozlowski-opensource approved these changes

@devversion devversion devversion left review comments

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker area: core Issues related to the framework runtime target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@crisbeto @eneajaho @jessicajaniuk @pkozlowski-opensource @devversion