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

o16n: primitives code location #51986

Closed
wants to merge 4 commits into from
Closed

o16n: primitives code location #51986

wants to merge 4 commits into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Oct 2, 2023 •
edited

Nothing to see here, Enea

eneajaho
eneajaho reviewed Oct 2, 2023
View reviewed changes
packages/core/src/render3/reactivity/signal.ts Outdated
* Update the current value by mutating it in-place, and
* notify any dependents.
*/
mutate(mutatorFn: (value: T) => void): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

So we are keeping the mutate fn? #51821

Copy link
Member

Choose a reason for hiding this comment

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

Probably not :-)

rhutchison
rhutchison reviewed Oct 2, 2023
View reviewed changes
packages/core/src/render3/reactivity/signal.ts
set(value: T): void;

/**
* Update the value of the signal based on itfs current value, and
Copy link

Choose a reason for hiding this comment

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

itfs?

@alxhub alxhub force-pushed the o16n/code-location branch 6 times, most recently from e039357 to 59d1425 Compare October 4, 2023 17:02
@alxhub alxhub marked this pull request as ready for review October 4, 2023 18:23
pkozlowski-opensource
pkozlowski-opensource approved these changes Oct 5, 2023
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

Reviewed-for: public-api
Reviewed-for: fw-core

@alxhub alxhub force-pushed the o16n/code-location branch 4 times, most recently from d7bb484 to 51824c3 Compare October 6, 2023 17:56
@angular-robot angular-robot bot added the detected: breaking change PR contains a commit with a breaking change label Oct 6, 2023
@alxhub alxhub force-pushed the o16n/code-location branch 2 times, most recently from 08b73dc to 8c065e2 Compare October 6, 2023 18:47
@alxhub alxhub added action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Oct 6, 2023
@alxhub
Copy link
Member Author

alxhub commented Oct 6, 2023

Caretaker: This will need cl/571389475 in the sync.

alxhub and others added 4 commits October 6, 2023 13:21
@alxhub
refactor(core): extract signals API away from the 'signals' package
7a9cfe9 
This commit refactors the signals API surface of Angular out of the
//packages/core/src/signals package. This is done in preparation of moving
the core signals package into a new 'primitives' package that's decoupled
from the public API.
@alxhub
refactor: move signals code into primitives package
ea8739c 
This commit reorganizes the Angular code a bit, and moves signals into a
newly defined `@angular/core/primitives` location. This will be used inside
g3 to allow non-Angular targets to depend on the signals core without
incurring a dependency on the whole framework.
@alxhub
refactor(core): add consumerOnSignalRead hook to ReactiveNode
c4de5df 
This hook allows a consumer to create `ReactiveNode`s with custom behavior
when signals are read.
@pkozlowski-opensource @alxhub
fix(core): drop mutate function from the signals public API (angular#…
087eb92 
…51821)

This change removes the `mutate` method from the `WritableSignal` interface and
completely drops it from the public API surface.

The initial API proposal for Angular signals included the mutate method, allowing
in-place modification of JS objects, without changing their references (identity).
This was based on the reasoning that identity change on modification is not necessary
as we can send the “modified” notification through the signals graph.
Unfortunately the signal-specific change notification is lost as soon as we read
signal value outside of a reactive context (outside of a reactive graph).
In other words - any code outside of the Angular signals library can’t know
that an object is modified.

Secondly, to make the mutate method work, we’ve defaulted the signal value equality function
to the one that considers non-primitive values as always different.
This is unfortunate for people working with immutable data structures
(this is notably the case for the popular state management libraries)
as the default equality function de-optimizes memoization in computed,
making the application less performant.

Given the above reasons we prefer to remove the mutate method in the signals library -
at least for now. There are just too many sharp edges and tradeoffs that we don’t fully
understand yet.

BREAKING CHANGE:

The  `mutate` method was removed from the `WritableSignal` interface and completely
dropped from the public API surface. As an alternative please use the update method and
make immutable changes to the object.

Example before:

```typescript
items.mutate(itemsArray => itemsArray.push(newItem));
```

Example after:

```typescript
items.update(itemsArray => [itemsArray, …newItem]);
```
@alxhub
Copy link
Member Author

alxhub commented Oct 6, 2023

Caretaker:

  • pullapprove disabled (code was already approved, partially in another PR)
  • this change requires a g3 patch: cl/571389475
  • the CI failure is due to the symbol test for defer which is already fixed

@atscott atscott added target: major This PR is targeted for the next major release and removed target: minor This PR is targeted for the next minor release labels Oct 6, 2023
@atscott
Copy link
Contributor

atscott commented Oct 6, 2023

This PR was merged into the repository by commit 00128e3.

@atscott atscott closed this in b91d143 Oct 6, 2023
atscott pushed a commit that referenced this pull request Oct 6, 2023
@alxhub @atscott
refactor: move signals code into primitives package (#51986)
8f5cbcc 
This commit reorganizes the Angular code a bit, and moves signals into a
newly defined `@angular/core/primitives` location. This will be used inside
g3 to allow non-Angular targets to depend on the signals core without
incurring a dependency on the whole framework.

PR Close #51986
atscott pushed a commit that referenced this pull request Oct 6, 2023
@alxhub @atscott
refactor(core): add consumerOnSignalRead hook to ReactiveNode (#5…
8914eaf 
…1986)

This hook allows a consumer to create `ReactiveNode`s with custom behavior
when signals are read.

PR Close #51986
atscott pushed a commit that referenced this pull request Oct 6, 2023
@pkozlowski-opensource @atscott
fix(core): drop mutate function from the signals public API (#51821) (#…
00128e3 
…51986)

This change removes the `mutate` method from the `WritableSignal` interface and
completely drops it from the public API surface.

The initial API proposal for Angular signals included the mutate method, allowing
in-place modification of JS objects, without changing their references (identity).
This was based on the reasoning that identity change on modification is not necessary
as we can send the “modified” notification through the signals graph.
Unfortunately the signal-specific change notification is lost as soon as we read
signal value outside of a reactive context (outside of a reactive graph).
In other words - any code outside of the Angular signals library can’t know
that an object is modified.

Secondly, to make the mutate method work, we’ve defaulted the signal value equality function
to the one that considers non-primitive values as always different.
This is unfortunate for people working with immutable data structures
(this is notably the case for the popular state management libraries)
as the default equality function de-optimizes memoization in computed,
making the application less performant.

Given the above reasons we prefer to remove the mutate method in the signals library -
at least for now. There are just too many sharp edges and tradeoffs that we don’t fully
understand yet.

BREAKING CHANGE:

The  `mutate` method was removed from the `WritableSignal` interface and completely
dropped from the public API surface. As an alternative please use the update method and
make immutable changes to the object.

Example before:

```typescript
items.mutate(itemsArray => itemsArray.push(newItem));
```

Example after:

```typescript
items.update(itemsArray => [itemsArray, …newItem]);
```

PR Close #51986
@alxhub alxhub mentioned this pull request Oct 8, 2023
Closed
@JeanMeche JeanMeche mentioned this pull request Oct 10, 2023
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 Nov 6, 2023
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@alxhub @ChellappanRajan
refactor(core): extract signals API away from the 'signals' package (a…
2f07b56 
…ngular#51986)

This commit refactors the signals API surface of Angular out of the
//packages/core/src/signals package. This is done in preparation of moving
the core signals package into a new 'primitives' package that's decoupled
from the public API.

PR Close angular#51986
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@alxhub @ChellappanRajan
refactor: move signals code into primitives package (angular#51986)
b406c37 
This commit reorganizes the Angular code a bit, and moves signals into a
newly defined `@angular/core/primitives` location. This will be used inside
g3 to allow non-Angular targets to depend on the signals core without
incurring a dependency on the whole framework.

PR Close angular#51986
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@alxhub @ChellappanRajan
refactor(core): add consumerOnSignalRead hook to ReactiveNode (an…
b737430 
…gular#51986)

This hook allows a consumer to create `ReactiveNode`s with custom behavior
when signals are read.

PR Close angular#51986
ChellappanRajan pushed a commit to ChellappanRajan/angular that referenced this pull request Jan 23, 2024
@pkozlowski-opensource @ChellappanRajan
fix(core): drop mutate function from the signals public API (angular#…
33f5094 
…51821) (angular#51986)

This change removes the `mutate` method from the `WritableSignal` interface and
completely drops it from the public API surface.

The initial API proposal for Angular signals included the mutate method, allowing
in-place modification of JS objects, without changing their references (identity).
This was based on the reasoning that identity change on modification is not necessary
as we can send the “modified” notification through the signals graph.
Unfortunately the signal-specific change notification is lost as soon as we read
signal value outside of a reactive context (outside of a reactive graph).
In other words - any code outside of the Angular signals library can’t know
that an object is modified.

Secondly, to make the mutate method work, we’ve defaulted the signal value equality function
to the one that considers non-primitive values as always different.
This is unfortunate for people working with immutable data structures
(this is notably the case for the popular state management libraries)
as the default equality function de-optimizes memoization in computed,
making the application less performant.

Given the above reasons we prefer to remove the mutate method in the signals library -
at least for now. There are just too many sharp edges and tradeoffs that we don’t fully
understand yet.

BREAKING CHANGE:

The  `mutate` method was removed from the `WritableSignal` interface and completely
dropped from the public API surface. As an alternative please use the update method and
make immutable changes to the object.

Example before:

```typescript
items.mutate(itemsArray => itemsArray.push(newItem));
```

Example after:

```typescript
items.update(itemsArray => [itemsArray, …newItem]);
```

PR Close angular#51986
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rhutchison rhutchison rhutchison left review comments

@eneajaho eneajaho eneajaho left review comments

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

@josephperrott josephperrott Awaiting requested review from josephperrott

Assignees
No one assigned
Labels
action: merge The PR is ready for merge by the caretaker detected: breaking change PR contains a commit with a breaking change merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note PullApprove: disable target: major This PR is targeted for the next major release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@alxhub @atscott @pkozlowski-opensource @rhutchison @eneajaho