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): return a readonly signal on asReadonly. #54706

Closed
wants to merge 1 commit into from
Closed

fix(core): return a readonly signal on asReadonly. #54706

wants to merge 1 commit into from

Conversation

JeanMeche
Copy link
Member

Previous asReadonly() returned the signal value and not the signal itself.

Fixes #54704

@pkozlowski-opensource pkozlowski-opensource added area: core Issues related to the framework runtime target: patch This PR is targeted for the next patch release labels Mar 5, 2024
@ngbot ngbot bot added this to the Backlog milestone Mar 5, 2024
@JeanMeche JeanMeche marked this pull request as ready for review March 5, 2024 14:25
@pullapprove pullapprove bot requested a review from atscott March 5, 2024 14:25
pkozlowski-opensource
pkozlowski-opensource requested changes Mar 5, 2024
View reviewed changes
Copy link
Member

@pkozlowski-opensource pkozlowski-opensource left a comment •
edited

Choose a reason for hiding this comment

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

Please add a stronger test

@JeanMeche JeanMeche force-pushed the chore/model-readonly branch 2 times, most recently from 9d60335 to 3bf96fc Compare March 5, 2024 15:13
@pkozlowski-opensource pkozlowski-opensource removed the request for review from atscott March 5, 2024 18:07
@pkozlowski-opensource pkozlowski-opensource added the action: merge The PR is ready for merge by the caretaker label Mar 5, 2024
@jnizet
Copy link
Contributor

jnizet commented Mar 6, 2024

Shouldn't the commit message start with fix(core)?

@pkozlowski-opensource
Copy link
Member

Shouldn't the commit message start with fix(core)?

It actually should - great catch @jnizet - I will fix it while merging

@pkozlowski-opensource pkozlowski-opensource added merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed and removed merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed labels Mar 6, 2024
@pkozlowski-opensource
Copy link
Member

@JeanMeche it looks like this one will need a rebase - also, could you amend the commit message (as suggested by @jnizet ) while doing so? Thnx!

@pkozlowski-opensource pkozlowski-opensource added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed action: merge The PR is ready for merge by the caretaker labels Mar 6, 2024
@JeanMeche JeanMeche force-pushed the chore/model-readonly branch 3 times, most recently from a15e682 to 1c60adc Compare March 6, 2024 12:09
@JeanMeche JeanMeche added action: merge The PR is ready for merge by the caretaker and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Mar 6, 2024
@JeanMeche
fix(core): return a readonly signal on asReadonly.
fd2c620 
Previous `asReadonly()` returned the signal value and not the signal itself.

Fixes angular#54704
@JeanMeche JeanMeche changed the title refactor(core): return a readonly signal on asReadonly. fix(core): return a readonly signal on asReadonly. Mar 6, 2024
@JeanMeche JeanMeche mentioned this pull request Mar 6, 2024
Closed
@JeanMeche JeanMeche added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Mar 6, 2024
@pkozlowski-opensource pkozlowski-opensource removed the merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed label Mar 6, 2024
@pkozlowski-opensource
Copy link
Member

This PR was merged into the repository by commit 7243c70.

@JeanMeche JeanMeche mentioned this pull request Mar 6, 2024
Closed
@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 6, 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

@alxhub alxhub alxhub approved these changes

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.

ModelSignal.asReadonly() returns value instead of readonly signal
4 participants
@JeanMeche @jnizet @pkozlowski-opensource @alxhub