Skip to content

feat(core): introduce the reactive linkedSignal - #58189

Closed
pkozlowski-opensource wants to merge 1 commit into
angular:mainfrom
pkozlowski-opensource:linked_signal
Closed

feat(core): introduce the reactive linkedSignal#58189
pkozlowski-opensource wants to merge 1 commit into
angular:mainfrom
pkozlowski-opensource:linked_signal

Conversation

@pkozlowski-opensource

@pkozlowski-opensource pkozlowski-opensource commented Oct 14, 2024

Copy link
Copy Markdown
Member

This change introduces the new reactive primitive: linkedSignal.

A linkedSignal represents state (hence the signal in the name) that is reset based on the provided computation. Conceptually it is a state that is maintained / valid only in the context of another source signal (context is deteremined by a computation).

@angular-robot angular-robot Bot added detected: feature PR contains a feature commit area: core Issues related to the framework runtime labels Oct 14, 2024
@ngbot ngbot Bot added this to the Backlog milestone Oct 14, 2024
@pkozlowski-opensource pkozlowski-opensource added the target: minor This PR is targeted for the next minor release label Oct 14, 2024
@angular-robot angular-robot Bot added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Oct 14, 2024
@eneajaho eneajaho mentioned this pull request Oct 14, 2024
Closed
@angular-robot angular-robot Bot added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Oct 14, 2024
@dostarora97

dostarora97 commented Oct 15, 2024

Copy link
Copy Markdown

Sorry if I’m missing something obvious here.
But how is this different from computed() signal ?

@Armenvardanyan95

Armenvardanyan95 commented Oct 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Sorry if I’m missing something obvious here. But how is this different from computed() signal ?

@dostarora97 This will be writable. Computed signals are only readable, so they completely depend on their tracked signals. With this, you can set your own values, but it will be overwritten if the source signal changes its value

@angular-robot angular-robot Bot added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Oct 15, 2024
@angular-robot angular-robot Bot added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Oct 15, 2024
@ngbot ngbot Bot modified the milestone: Backlog Oct 15, 2024
@angular-robot angular-robot Bot added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Oct 15, 2024
alxhub
alxhub reviewed Oct 15, 2024
View reviewed changes
Comment thread goldens/public-api/core/primitives/signals/index.api.md Outdated
Comment thread packages/core/src/render3/reactivity/linked_signal.ts Outdated
Comment thread packages/core/src/render3/reactivity/linked_signal.ts Outdated
Comment thread packages/core/primitives/signals/src/computed.ts Outdated
@angular-robot angular-robot Bot added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Oct 16, 2024
@angular-robot angular-robot Bot added area: core Issues related to the framework runtime and removed area: core Issues related to the framework runtime labels Oct 16, 2024
@ngbot ngbot Bot modified the milestone: Backlog Oct 16, 2024
@pkozlowski-opensource
pkozlowski-opensource marked this pull request as ready for review October 16, 2024 16:35
@pkozlowski-opensource pkozlowski-opensource added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 16, 2024
@pullapprove pullapprove Bot added requires: TGP This PR requires a passing TGP before merging is allowed and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Oct 16, 2024
chrisrocco
chrisrocco reviewed Oct 16, 2024
View reviewed changes

@chrisrocco chrisrocco left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I believe it's possible to construct this API using existing primitives. Was this approach considered, and/or what are the benefits of creating a new subclass of ReactiveNode instead? Performance? DevTools?

Something like this (plus the other features) -

function linkedSignal(src: Signal) {
  const wrapped = () => signal(src());
  const getter = () => wrapped()[0]();
  const setter = (next) => wrapped()[1](next);
  getter.set = setter;
  return getter;
}

@JeanMeche JeanMeche mentioned this pull request Oct 17, 2024
Open
alxhub
alxhub approved these changes Oct 17, 2024
View reviewed changes

@alxhub alxhub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@pkozlowski-opensource

pkozlowski-opensource commented Oct 17, 2024

Copy link
Copy Markdown
Member Author

@chrisrocco there are some complications with implementing this purely in the user-land: mostly around previous values and equality. Will follow up with you offline.

@pkozlowski-opensource
feat(core): introduce the reactive linkedSignal
ce7d8c7 
This change introduces the new reactive primitive: linkedSignal.

A linkedSignal represents state (hence the signal in the name)
that is reset based on the provided computation. Conceptually
it is a state that is maintained / valid only in the context of
another source signal (context is deteremined by a computation).

Closes angular#55673
@pullapprove pullapprove Bot removed the requires: TGP This PR requires a passing TGP before merging is allowed label Oct 17, 2024
alxhub
alxhub approved these changes Oct 17, 2024
View reviewed changes

@alxhub alxhub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed-for: public-api

thePunderWoman
thePunderWoman approved these changes Oct 17, 2024
View reviewed changes

@thePunderWoman thePunderWoman left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

reviewed-for: public-api, fw-core

AndrewKushnir
AndrewKushnir approved these changes Oct 17, 2024
View reviewed changes

@AndrewKushnir AndrewKushnir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@devversion

devversion commented Oct 18, 2024

Copy link
Copy Markdown
Member

This PR was merged into the repository by commit 8311f00.

The changes were merged into the following branches: main

@Ookamini95

Ookamini95 commented Nov 8, 2024 •
edited
Loading

Copy link
Copy Markdown

Not the best programmer to hear from, but i belive "linked" would keep close to the style of "signal" and "computed". Nothing wrong with longer names, but if you really want to go for it why not a more intuitive name like "writableComputed"? What is the reasoning behind "linkedSignal" naming convention?

@angular-automatic-lock-bot

angular-automatic-lock-bot Bot commented Dec 9, 2024

Copy link
Copy Markdown

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@thePunderWoman thePunderWoman thePunderWoman approved these changes
@alxhub alxhub alxhub approved these changes
@AndrewKushnir AndrewKushnir AndrewKushnir approved these changes
+1 more reviewer
@chrisrocco chrisrocco chrisrocco left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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: minor This PR is targeted for the next minor release

Projects

None yet

Milestone

v19 Feature Freeze Candidates

Development

Successfully merging this pull request may close these issues.

9 participants

@pkozlowski-opensource @dostarora97 @Armenvardanyan95 @devversion @Ookamini95 @thePunderWoman @alxhub @chrisrocco @AndrewKushnir