Skip to content

feat(core): add "kind" to signal prototype nodes#58333

Closed
AleksanderBodurri wants to merge 1 commit intoangular:mainfrom
AleksanderBodurri:export-computed-node
Closed

feat(core): add "kind" to signal prototype nodes#58333
AleksanderBodurri wants to merge 1 commit intoangular:mainfrom
AleksanderBodurri:export-computed-node

Conversation

@AleksanderBodurri
Copy link
Copy Markdown
Member

@AleksanderBodurri AleksanderBodurri commented Oct 23, 2024 •
edited
Loading

This would enable debug APIs to determine the kind of a signal given a node for #57074

@AleksanderBodurri AleksanderBodurri added the area: core Issues related to the framework runtime label Oct 23, 2024
@ngbot ngbot bot added this to the Backlog milestone Oct 23, 2024
@pullapprove pullapprove bot requested a review from eduhmc October 23, 2024 18:25
@pullapprove pullapprove bot added the requires: TGP This PR requires a passing TGP before merging is allowed label Oct 23, 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 23, 2024
@AleksanderBodurri AleksanderBodurri added the target: minor This PR is targeted for the next minor release label Oct 24, 2024
@angular-robot angular-robot bot added the detected: feature PR contains a feature commit label Nov 6, 2024
@AleksanderBodurri AleksanderBodurri changed the title refactor(core): export COMPUTED_NODE @AleksanderBodurri feat(core): add "kind" to signal prototype nodes Nov 6, 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 Nov 6, 2024
@ngbot ngbot bot modified the milestone: Backlog Nov 6, 2024
@AleksanderBodurri AleksanderBodurri changed the title @AleksanderBodurri feat(core): add "kind" to signal prototype nodes feat(core): add "kind" to signal prototype nodes Nov 6, 2024
dgp1130
dgp1130 approved these changes Nov 11, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@dgp1130 dgp1130 left a comment

Choose a reason for hiding this comment

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

NOTE: resource uses signal under the hood, so it will have kind: 'signal', I think that's fine for now, but just FYI.

alxhub
alxhub approved these changes Nov 11, 2024
View reviewed changes
packages/core/primitives/signals/src/graph.ts Outdated
Copy link
Copy Markdown
Member

@alxhub alxhub Nov 11, 2024

Choose a reason for hiding this comment

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

We can make this required, and specify kind: 'unknown' in REACTIVE_NODE.

packages/core/src/render3/reactive_lview_consumer.ts Outdated
Copy link
Copy Markdown
Member

@alxhub alxhub Nov 11, 2024

Choose a reason for hiding this comment

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

Angular and primitives code changes can't go into the same commit sadly (though this LGTM).

@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 Nov 20, 2024
@ngbot ngbot bot modified the milestone: Backlog Nov 20, 2024
@angular-robot angular-robot bot removed the area: core Issues related to the framework runtime label Nov 20, 2024
AndrewKushnir
AndrewKushnir approved these changes Nov 27, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@AndrewKushnir AndrewKushnir left a comment

Choose a reason for hiding this comment

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

Reviewed-for: public-api

@AndrewKushnir AndrewKushnir removed the request for review from thePunderWoman November 27, 2024 05:21
@thePunderWoman thePunderWoman requested review from tbondwilkinson and removed request for eduhmc November 27, 2024 15:21
mturco
mturco approved these changes Nov 27, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@mturco mturco left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Reviewed-for: public-api
reviewed-for: primitives-shared

goldens/public-api/core/primitives/signals/index.api.md Outdated
Copy link
Copy Markdown
Contributor

@mturco mturco Nov 27, 2024

Choose a reason for hiding this comment

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

I assume there's a reason the type is string here rather than 'signal' | 'computed' | ...?

Copy link
Copy Markdown
Member Author

@AleksanderBodurri AleksanderBodurri Dec 2, 2024

Choose a reason for hiding this comment

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

If the Angular framework wants to create a node type and set kind to 'some-new-node-type' we would have to add that to the type contract in the primitive signals library. Having the type contract be less strict here lets us avoid this coupling and lets primitives/signals library consumers define their own "kind" of nodes.

Copy link
Copy Markdown
Contributor

@mturco mturco Dec 2, 2024

Choose a reason for hiding this comment

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

Good point! 👍

@thePunderWoman thePunderWoman removed the request for review from tbondwilkinson November 27, 2024 17:02
@thePunderWoman
Copy link
Copy Markdown
Contributor

thePunderWoman commented Nov 27, 2024

@AleksanderBodurri Looks like other than fixing lint and a presubmit, this is good to go.

@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 Nov 28, 2024
@angular-robot angular-robot bot requested a review from mturco November 28, 2024 01:29
@AleksanderBodurri
Copy link
Copy Markdown
Member Author

AleksanderBodurri commented Nov 28, 2024

ahh missed that linting was failing, thanks @thePunderWoman 🙏

@AleksanderBodurri
refactor(core): add "kind" to signal prototype nodes
0836aff 
Enables debug APIs to determine the kind of a signal given a node.
@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 Dec 2, 2024
@ngbot ngbot bot modified the milestone: Backlog Dec 2, 2024
@AleksanderBodurri AleksanderBodurri added the action: merge The PR is ready for merge by the caretaker label Dec 2, 2024
@pkozlowski-opensource
Copy link
Copy Markdown
Member

pkozlowski-opensource commented Dec 3, 2024

"green" TGP

@pkozlowski-opensource pkozlowski-opensource removed the request for review from mturco December 3, 2024 14:11
@pkozlowski-opensource pkozlowski-opensource added target: patch This PR is targeted for the next patch release target: minor This PR is targeted for the next minor release and removed target: minor This PR is targeted for the next minor release target: patch This PR is targeted for the next patch release labels Dec 3, 2024
@pkozlowski-opensource
Copy link
Copy Markdown
Member

pkozlowski-opensource commented Dec 3, 2024

This PR was merged into the repository by commit fa0a9a5.

The changes were merged into the following branches: main

AleksanderBodurri added a commit to AleksanderBodurri/angular that referenced this pull request Dec 3, 2024
@AleksanderBodurri
feat(core): set kind field on template and effect nodes
8896def 
Builds on angular#58333 by assigning the kind field to reactive nodes that are not in the `core/primitives` package.
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Jan 3, 2025

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

@mturco mturco mturco approved these changes

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

@alxhub alxhub alxhub approved these changes

@dgp1130 dgp1130 dgp1130 approved these changes

@AndrewKushnir AndrewKushnir AndrewKushnir 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 requires: TGP This PR requires a passing TGP before merging is allowed 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.

7 participants

@AleksanderBodurri @thePunderWoman @pkozlowski-opensource @mturco @alxhub @dgp1130 @AndrewKushnir