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.

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

refactor(core): prototype of signals, a reactive primitive for Angular #49091

Closed
wants to merge 1 commit into from

Conversation

alxhub
Copy link
Member

@alxhub alxhub commented Feb 15, 2023

For context, see this Github Discussion on our experiments with fine-grained reactivity in Angular

This commit checks in (but does not export) a prototype implementation of Angular Signals, along with its unit test suite and a README explaining the algorithms used.

Signals are not a new concept in the framework space, but there are many different flavors of implementations. These differ radically both in terms of public API as well as behavioral details (such as eager vs lazy computation, batching behavior, equality, cleanup, nesting, etc).

This commit comprises a bespoke implementation that we've designed to best meet Angular's needs, especially when it comes to compatibility and flexibility of use within existing applications.

Many of the API features of this implementation of signals, as well as the larger direction of reactivity in Angular, will be discussed in future RFCs.

🚦

@alxhub alxhub added action: review The PR is still awaiting reviews from at least one requested reviewer core: reactivity Work related to fine-grained reactivity in the core framework labels Feb 15, 2023
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!

@pkozlowski-opensource pkozlowski-opensource added target: major This PR is targeted for the next major release 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 Feb 15, 2023
@alxhub
Copy link
Member Author

alxhub commented Feb 15, 2023

LGTM!

Wow, that was a fast code review, @pkozlowski-opensource!

@pkozlowski-opensource
Copy link
Member

Wow, that was a fast code review, @pkozlowski-opensource!

Actually, isn't it one of the slowest? I was looking at the variants of this code for weeks already :-)

@alxhub alxhub requested a review from jelbourn February 15, 2023 21:36
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

Overall looks good, just a peppering of minor comment/naming nits

packages/core/src/signals/src/api.ts Show resolved Hide resolved
packages/core/src/signals/src/api.ts Outdated Show resolved Hide resolved
packages/core/src/signals/src/api.ts Outdated Show resolved Hide resolved
packages/core/src/signals/src/api.ts Outdated Show resolved Hide resolved
packages/core/src/signals/src/api.ts Outdated Show resolved Hide resolved
packages/core/src/signals/src/untrack.ts Outdated Show resolved Hide resolved
packages/core/src/signals/src/untrack.ts Outdated Show resolved Hide resolved
packages/core/test/signals/computed_spec.ts Show resolved Hide resolved
packages/core/test/signals/glitch_free_spec.ts Outdated Show resolved Hide resolved
packages/core/test/signals/regression_spec.ts Outdated Show resolved Hide resolved
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of nonblocking comments

@@ -0,0 +1,248 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

Non blocking, but consider changing/reorganizing in a follow-up PR. IMO file names should always describe the contents, so having a file named internal.ts is on par with having a file named functions.ts or classes.ts


import {WeakRef} from './weak_ref';

export type ProducerId = number&{__producer: true};
Copy link
Member

Choose a reason for hiding this comment

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

Non blocking, but a comment here would be useful

This commit checks in (but does not export) a prototype implementation of
Angular Signals, along with its unit test suite and a README explaining the
algorithms used.

Signals are not a new concept in the framework space, but there are many
different flavors of implementations. These differ radically both in terms
of public API as well as behavioral details (such as eager vs lazy
computation, batching behavior, equality, cleanup, nesting, etc).

This commit comprises a bespoke implementation that we've designed to best
meet Angular's needs, especially when it comes to compatibility and
flexibility of use within existing applications.

Many of the API features of this implementation of signals, as well as the
larger direction of reactivity in Angular, will be discussed in future RFCs.

Co-Authored-By: Pawel Kozlowski <pkozlowski.opensource@gmail.com>
@alxhub
Copy link
Member Author

alxhub commented Feb 21, 2023

This PR was merged into the repository by commit 02cd490.

@yangfan3211
Copy link

can I remove zone when I use Signal?

const counter = signal(0);

// Automatically updates when `counter` changes:
const isEven = computed(() => counter() % 2 === 0);
Copy link

Choose a reason for hiding this comment

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

@alxhub If we don't preprocess function in computed at compile-time or use some form of reflection to atomically notify only affected signals, won't it require O(n) change checks, where n is a quantity of signals in application, effectively making it have same algorithmic complexity as ChangeDetectionStrategy.Default and making it harmful for angular ecosystem?(with signals there will be 2 ways to run change detection in O(n), using signals or ChangeDetectionStrategy.Default(which is simpler to do than signals), and 1 way to run change detection in ~O(1) - using rxjs and async pipe)

Choose a reason for hiding this comment

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

@alxhub Oh, sorry, now I understand. We know for sure who is current consumer(because we track it in activeConsumer) and current producer(we can track it in code), so we have all the information needed to create dependency graph which will run atomically in O(1). I think we should mention explicitly for people like me why it runs in O(1)

@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 May 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker core: reactivity Work related to fine-grained reactivity in the core framework target: major This PR is targeted for the next major release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet