Batch of signal-related updates #49529

alxhub · 2023-03-22T02:51:41Z

(more coming)

AndrewKushnir · 2023-03-22T04:34:07Z

packages/core/src/di/contextual.ts

+ *
+ * @publicApi
+ */
+export function assertInInjectionContext(debugFn: Function): void {


Just as an idea: we can consider exposing a function like isInInjectionContext, which would return true/false. Using this function developer may be able to implement the necessary assertions and/or other logic as needed (and it's a bit easier to work with vs a function that throws).

I don't think we should.

APIs shouldn't sneakily change their behavior depending on whether they're in an injection context. The "right" thing to do is throw if called from the wrong context.

Having one public API function to check the injection context and throw allows us to give a very standard error message with good guidance on how to solve the problem across all of the APIs that rely on inject.

packages/core/src/render3/reactivity/effect.ts

pkozlowski-opensource · 2023-03-28T10:50:57Z

packages/core/src/signals/src/signal.ts

  // Casting here is required for g3.
  const signalFn = createSignalFromFunction(signalNode.signal.bind(signalNode), {
                     set: signalNode.set.bind(signalNode),
                     update: signalNode.update.bind(signalNode),
                     mutate: signalNode.mutate.bind(signalNode),
-                   }) as unknown as SettableSignal<T>;
+                   }) as unknown as WritableSignal<T>;


super-nit: we've got the // Casting here is required for g3 which is pretty enigmatic - could we expand it to say "why" it is needed in g3?

pkozlowski-opensource · 2023-03-28T10:56:34Z

packages/core/src/signals/src/signal.ts

@@ -96,8 +96,8 @@ class WritableSignalImpl<T> implements Producer {
 * @developerPreview
 */
 export function signal<T>(
-    initialValue: T, equal: ValueEqualityFn<T> = defaultEquals): WritableSignal<T> {
-  const signalNode = new WritableSignalImpl(initialValue, equal);
+    initialValue: T, options?: {equal?: ValueEqualityFn<T>}): WritableSignal<T> {


nit / thinking out loud: should we introduce a specific type for options? Or is it just too much hassle / too many symbols in the public API surface?

Asking since I can see the CreateEffectOptions in the later commit.

pkozlowski-opensource

@alxhub I went through this PR and most changes LGTM. Is there anything that you think is still needed to get this PR over the finish line?

One thing I'm seeing as still needing work is the G3 failures that seem legit (hence adding cleanup label). Also there are open comments from @AndrewKushnir .

packages/core/src/render3/reactivity/effect.ts

pkozlowski-opensource · 2023-03-28T14:48:10Z

packages/core/test/render3/reactivity_spec.ts

+import {bootstrapApplication} from '@angular/platform-browser';
+import {withBody} from '@angular/private/testing';
+
+describe('effects', () => {


existing effect tests should be moved here as well, right? Otherwise we start to have 2 implementations in 2 separate places.

We have two sets of tests:

tests for the signal lib itself, independent of FW

tests that validate FW integration with signals

packages/core/src/render3/instructions/shared.ts

dylhunn

reviewed-for: fw-core, public-api

This commit adds an assertion function to the public API, which allows authors of functions which rely on `inject` to validate that they're being called with the right context. This mostly produces a nicer error message than calling `inject()` and relying on Angular's default error message for that.

This commit renames the `SettableSignal` interface to align with recent naming changes. `WritableSignal` is considered a more suitable name since it supports more mutation operations than just directly setting.

This commit switches the `signal` and `computed` APIs to accept an optional options argument as their second argument, instead of an equality function directly. The equality function has moved to an option in the options argument.

…tion The `effect` implemented in the signal library is useful for testing but does not integrate with Angular. This commit moves that code to the actual framework package and integrates it with automatic cleanup via `DestroyRef`. A simpler effect implementation is used in the signal tests to test the `Watch` primitive. Further commits will update the scheduling to tie effects together with change detection.

This commit adds a `DeepReadonly` type to the signals API, and makes signal getters return an immutable version of their value type. This doesn't prevent all mutation but adds some friction against modifying the values within signals outside of the proper mutation APIs.

Previously, effects were queued in the global microtask queue and executed directly. This had an undesired consequence: whichever effect scheduled first determined the zone in which all other effects would run, *and* that zone depended on where the signal happened to be set which triggered that first effect. This behavior would be extremely unpredictable. This commit adds zone awareness to the effect API. effects now capture the current zone when they're created, and this zone is used to run the effect callback regardless of which zone set the signal.

Previously the signals implementation maintained separate interfaces for `Producer` and `Consumer` nodes, with implementers choosing to implement one or both interfaces. Operations defined against those interfaces were exposed as importable functions to be called with the object implementing the relevant contract as the first argument. This commit refactors the implementation to merge both abstractions into a single `ReactiveNode` base class, which represents both producers and consumers in the graph. Implementers choose to interact with a subset of the `ReactiveNode` API depending on their role in the graph. Operations are now available as protected methods on the base class, instead of separate functions.

Signal functions are marked with a `[SIGNAL]` symbol property, which was previously set to `true`. This commit changes the `[SIGNAL]` property to have the `ReactiveNode` as its value instead. This doesn't change the behavior of the signal library itself, but exposes the signal internals in a way where developer tooling can look at any signal and interact with its dependency graph.

atscott · 2023-03-29T01:48:00Z

This PR was merged into the repository by commit ef149de.

This commit renames the `SettableSignal` interface to align with recent naming changes. `WritableSignal` is considered a more suitable name since it supports more mutation operations than just directly setting. PR Close #49529

…nt (#49529) This commit switches the `signal` and `computed` APIs to accept an optional options argument as their second argument, instead of an equality function directly. The equality function has moved to an option in the options argument. PR Close #49529

…tion (#49529) The `effect` implemented in the signal library is useful for testing but does not integrate with Angular. This commit moves that code to the actual framework package and integrates it with automatic cleanup via `DestroyRef`. A simpler effect implementation is used in the signal tests to test the `Watch` primitive. Further commits will update the scheduling to tie effects together with change detection. PR Close #49529

…#49529) This commit adds a `DeepReadonly` type to the signals API, and makes signal getters return an immutable version of their value type. This doesn't prevent all mutation but adds some friction against modifying the values within signals outside of the proper mutation APIs. PR Close #49529

) Previously, effects were queued in the global microtask queue and executed directly. This had an undesired consequence: whichever effect scheduled first determined the zone in which all other effects would run, *and* that zone depended on where the signal happened to be set which triggered that first effect. This behavior would be extremely unpredictable. This commit adds zone awareness to the effect API. effects now capture the current zone when they're created, and this zone is used to run the effect callback regardless of which zone set the signal. PR Close #49529

Previously the signals implementation maintained separate interfaces for `Producer` and `Consumer` nodes, with implementers choosing to implement one or both interfaces. Operations defined against those interfaces were exposed as importable functions to be called with the object implementing the relevant contract as the first argument. This commit refactors the implementation to merge both abstractions into a single `ReactiveNode` base class, which represents both producers and consumers in the graph. Implementers choose to interact with a subset of the `ReactiveNode` API depending on their role in the graph. Operations are now available as protected methods on the base class, instead of separate functions. PR Close #49529

…49529) Signal functions are marked with a `[SIGNAL]` symbol property, which was previously set to `true`. This commit changes the `[SIGNAL]` property to have the `ReactiveNode` as its value instead. This doesn't change the behavior of the signal library itself, but exposes the signal internals in a way where developer tooling can look at any signal and interact with its dependency graph. PR Close #49529

angular-automatic-lock-bot · 2023-04-29T00:07:46Z

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.}

pullapprove bot requested review from AndrewKushnir, atscott and dylhunn March 22, 2023 02:51

angular-robot bot added the detected: feature PR contains a feature commit label Mar 22, 2023

AndrewKushnir reviewed Mar 22, 2023

View reviewed changes

pullapprove bot requested a review from AndrewKushnir March 22, 2023 04:37

alxhub force-pushed the fw/reactivity/signal-api-updates branch 3 times, most recently from 440bedd to c010ed1 Compare March 22, 2023 14:55

alxhub mentioned this pull request Mar 22, 2023

Run effects in the zone in which they're created #49544

Closed

pkozlowski-opensource added the area: core Issues related to the framework runtime label Mar 22, 2023

ngbot bot added this to the Backlog milestone Mar 22, 2023

pkozlowski-opensource self-requested a review March 28, 2023 09:50

pkozlowski-opensource reviewed Mar 28, 2023

View reviewed changes

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 target: major This PR is targeted for the next major release labels Mar 28, 2023

pkozlowski-opensource reviewed Mar 28, 2023

View reviewed changes

packages/core/src/render3/reactivity/effect.ts Show resolved Hide resolved

pkozlowski-opensource reviewed Mar 28, 2023

View reviewed changes

alxhub force-pushed the fw/reactivity/signal-api-updates branch from c010ed1 to 0824e60 Compare March 28, 2023 19:35

alxhub added action: review The PR is still awaiting reviews from at least one requested reviewer 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 28, 2023

alxhub force-pushed the fw/reactivity/signal-api-updates branch 2 times, most recently from 4e50882 to f6bae6d Compare March 28, 2023 20:02

alxhub changed the title ~~Batch of signal-related API updates~~ Batch of signal-related updates Mar 28, 2023

pkozlowski-opensource mentioned this pull request Mar 28, 2023

Refactoring of the signal library internals #49556

Closed

alxhub force-pushed the fw/reactivity/signal-api-updates branch from f6bae6d to aa1ce5c Compare March 28, 2023 21:42

alxhub commented Mar 28, 2023

View reviewed changes

packages/core/src/render3/instructions/shared.ts Outdated Show resolved Hide resolved

dylhunn approved these changes Mar 28, 2023

View reviewed changes

pullapprove bot requested a review from jessicajaniuk March 28, 2023 22:54

alxhub force-pushed the fw/reactivity/signal-api-updates branch 2 times, most recently from f87363b to 379e5e5 Compare March 28, 2023 23:40

alxhub added 6 commits March 28, 2023 17:46

refactor(core): rename SettableSignal to WritableSignal

9fae62c

This commit renames the `SettableSignal` interface to align with recent naming changes. `WritableSignal` is considered a more suitable name since it supports more mutation operations than just directly setting.

alxhub force-pushed the fw/reactivity/signal-api-updates branch from 379e5e5 to 11fb5e4 Compare March 29, 2023 00:49

alxhub added 2 commits March 28, 2023 18:09

alxhub force-pushed the fw/reactivity/signal-api-updates branch from 11fb5e4 to 1e8f12a Compare March 29, 2023 01:09

alxhub added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 29, 2023

alxhub removed request for jessicajaniuk and AndrewKushnir March 29, 2023 01:36

pullapprove bot requested review from jessicajaniuk and AndrewKushnir March 29, 2023 01:36

atscott closed this in 89d291c Mar 29, 2023

angular-automatic-lock-bot bot locked and limited conversation to collaborators Apr 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch of signal-related updates #49529

Batch of signal-related updates #49529

alxhub commented Mar 22, 2023

AndrewKushnir Mar 22, 2023

alxhub Mar 28, 2023

pkozlowski-opensource Mar 28, 2023

alxhub Mar 28, 2023

pkozlowski-opensource Mar 28, 2023 •

edited

alxhub Mar 28, 2023

pkozlowski-opensource left a comment •

edited

pkozlowski-opensource Mar 28, 2023

alxhub Mar 28, 2023

dylhunn left a comment •

edited

atscott commented Mar 29, 2023

angular-automatic-lock-bot bot commented Apr 29, 2023

Batch of signal-related updates #49529

Batch of signal-related updates #49529

Conversation

alxhub commented Mar 22, 2023

AndrewKushnir Mar 22, 2023

Choose a reason for hiding this comment

alxhub Mar 28, 2023

Choose a reason for hiding this comment

pkozlowski-opensource Mar 28, 2023

Choose a reason for hiding this comment

alxhub Mar 28, 2023

Choose a reason for hiding this comment

pkozlowski-opensource Mar 28, 2023 • edited

Choose a reason for hiding this comment

alxhub Mar 28, 2023

Choose a reason for hiding this comment

pkozlowski-opensource left a comment • edited

Choose a reason for hiding this comment

pkozlowski-opensource Mar 28, 2023

Choose a reason for hiding this comment

alxhub Mar 28, 2023

Choose a reason for hiding this comment

dylhunn left a comment • edited

Choose a reason for hiding this comment

atscott commented Mar 29, 2023

angular-automatic-lock-bot bot commented Apr 29, 2023

pkozlowski-opensource Mar 28, 2023 •

edited

pkozlowski-opensource left a comment •

edited

dylhunn left a comment •

edited