8000 Transform logic to extract signal debug names by AleksanderBodurri · Pull Request #57348 · angular/angular · GitHub
[go: up one dir, main page]
Skip to content

Transform logic to extract signal debug names #57348

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 occasiona 8000 lly send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Transform logic to extract signal debug names #57348

wants to merge 1 commit into from

Conversation

AleksanderBodurri
Copy link
Member
@AleksanderBodurri AleksanderBodurri commented Aug 12, 2024 •
edited
Loading

Implements a compiler transform that attempts to statically analyze variable names and apply them to usages of signal functions like signal, computed, effect, etc as debug names.

Related: #57074

Depends on: #57073

@AleksanderBodurri AleksanderBodurri added state: blocked area: compiler Issues related to `ngc`, Angular's template compiler labels Aug 12, 2024
@ngbot ngbot bot modified the milestone: Backlog Aug 12, 2024
@pullapprove pullapprove bot added requires: TGP This PR requires a passing TGP before merging is allowed labels Aug 12, 2024
@angular-robot angular-robot bot added detected: feature PR contains a feature commit area: core Issues related to the framework runtime labels Aug 12, 2024
dgp1130
dgp1130 reviewed Aug 12, 2024
View reviewed changes
josephperrott
josephperrott approved these changes 10000 Aug 13, 2024
View reviewed changes
Copy link
Member
@josephperrott josephperrott left a comment

Choose a reason for hiding this comment

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

LGTM

@AleksanderBodurri AleksanderBodurri force-pushed the signal-debug-name-transform branch from c3d0bf5 to fbbc8c0 Compare August 26, 2024 05:57
alxhub
alxhub reviewed Sep 24, 2024
View reviewed changes
Copy link
Member
@alxhub alxhub left a comment

Choose a reason for hiding this comment

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

First round of comments!

Additionally, in the commit history, use refactor commits for implementation details. A given feature (debug names for example) should have exactly 1 feat commit, which is the commit which actually makes the feature available for end users to use.

packages/compiler-cli/src/ngtsc/transform/src/implicit_signal_debug_name_transform.ts Outdated
]),
undefined,
ts.factory.createArrayLiteralExpression([
ts.factory.createObjectLiteralExpression(existingArgument.properties),
Copy link
Member

Choose a reason for hiding this comment

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

Same comment about ts.factory.update*.

Copy link
Member

Choose a reason for hiding this comment

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

This is still a bit sketchy to me. This creates a node without any context (e.g. sourcemaps).

Ideally, we wouldn't have to copy the user's code like this, but we should still use ts.update* instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

We discussed this briefly in the DevTools sync today. We can definitely update this object literal to just be existingArgument. How should this work for the duplicated node though?

We're effectively converting signal(0, { equals }) to signal(0, ...(ngDevMode ? [{ debugName: 'test', equals }] : [{ equals }])). That moves { equals } to the false case, but also duplicate its in the true case. I presume we just need to ts.factory.create*(...) for the true case and can't update the existing one because we need it for the false case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Tested over the weekend with ng new and the compiler cli code with the transform. Source maps are working as expected even with the transformation applied. I think this is working as expected now

@angular-robot angular-robot bot added area: core Issues related to the framework runtime and removed area: compiler Issues related to `ngc`, Angular's template compiler area: core Issues related to the framework runtime labels Sep 30, 2024
@ngbot ngbot bot modified the milestone: Backlog Sep 30, 2024
dgp1130
dgp1130 reviewed Oct 18, 2024
View reviewed changes
@AleksanderBodurri AleksanderBodurri force-pushed the signal-debug-name-transform branch from 252d2cd to 5f59f91 Compare October 19, 2024 19:34
@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 19, 2024
@AleksanderBodurri AleksanderBodurri force-pushed the signal-debug-name-transform branch 3 times, most recently from 43e0707 to 850cea2 Compare March 3, 2025 15:54
dgp1130
dgp1130 reviewed Mar 4, 2025
View reviewed changes
Copy link
Contributor
@dgp1130 dgp1130 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.

Thanks for working through all these changes, it's looking great!

No major concerns on my side, just a few minor suggestions and questions. I think a couple existing comments are still open too when you get a chance.

packages/compiler-cli/src/ngtsc/transform/src/implicit_signal_debug_name_transform.ts
program: ts.Program,
expression: ts.Expression,
): boolean {
const symbol = program.getTypeChecker().getSymbolAtLocation(expression);
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: @alxhub, do we care that this is using the type checker to resolve to the imported symbol?

I know we're trying to avoid the type checker for single-file transpilations, so I'm wondering if there's a better way or if this is ok because we're keeping it within the source file.

packages/compiler-cli/src/ngtsc/transform/src/implicit_signal_debug_name_transform.ts
}

// climb up the tree from the import specifier to the import declaration
const importSpecifier = declarations[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: What exactly does it mean to have multiple declarations? Are these shadowed declarations like:

import { signal } from '@angular/core';

function foo() {
  const signal = () => {};
  console.log(signal); // Has two declarations?
}

Copy link
Member
@alxhub alxhub Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

No, multiple declarations would be something like:

interface Foo {
  bar: string;
}

interface Foo {
  baz: number;
}

Multiple declarations really only applies to types. When looking for the declaration of a value, you can use symbol.valueDeclaration instead, since a value can only ever be defined once.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it safe to just look at the first declaration? I guess if it is a real signal function then it will always have just one declaration from the import?

@AleksanderBodurri AleksanderBodurri force-pushed the signal-debug-name-transform branch from 850cea2 to 7c96e70 Compare March 10, 2025 13:42
@AleksanderBodurri AleksanderBodurri force-pushed the signal-debug-name-transform branch 5 times, most recently from 1f03d73 to 57ec5c4 Compare March 24, 2025 11:39
@AleksanderBodurri AleksanderBodurri force-pushed the signal-debug-name-transform branch 7 times, most recently from af1f967 to f05cdea Compare March 31, 2025 17:55
@dgp1130 dgp1130 force-pushed the signal-debug-name-transform branch from f05cdea to 780dab1 Compare April 2, 2025 17:49
@AleksanderBodurri AleksanderBodurri force-pushed the signal-debug-name-transform branch from 780dab1 to 6c5f302 Compare April 7, 2025 15:10
@AleksanderBodurri AleksanderBodurri force-pushed the signal-debug-name-transform branch 2 times, most recently from f034848 to 52c3eb0 Compare April 21, 2025 05:31
@dgp1130
Copy link
Contributor
dgp1130 commented Apr 23, 2025

Passing TGP: http://test/OCL:749909832:BASE:750657379:1745431857014:f318b2d7

@AleksanderBodurri AleksanderBodurri force-pushed the signal-debug-name-transform branch from 52c3eb0 to e8acd3f Compare April 23, 2025 21:54
@AleksanderBodurri AleksanderBodurri force-pushed the signal-debug-name-transform branch from ad1b0ad to 57eaa4e Compare May 12, 2025 16:08
@dgp1130
Copy link
Contributor
dgp1130 commented May 14, 2025

TGP still looking good: http://test/OCL:758440362:BASE:758744767:1747244383957:e4a093db

@AleksanderBodurri AleksanderBodurri force-pushed the signal-debug-name-transform branch from 57eaa4e to 05a8db2 Compare May 15, 2025 05:51
@AleksanderBodurri
refactor(compiler-cli): implement transform to determine debugName fr…
0a905d2 
…om signal functions

Implements a compiler transform that attempts to statically analyze variable names and apply them to usages of signal functions like signal, computed, effect, etc.
@AleksanderBodurri AleksanderBodurri force-pushed the signal-debug-name-transform branch from 05a8db2 to 0a905d2 Compare May 16, 2025 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alxhub alxhub alxhub left review comments

@devversion devversion devversion left review comments

@dgp1130 dgp1130 dgp1130 left review comments

@josephperrott josephperrott josephperrott approved these changes

@ellenyuan ellenyuan Awaiting requested review from ellenyuan

Assignees
No one assigned
Labels
area: compiler Issues related to `ngc`, Angular's template compiler area: devtools
Projects
None yet
Milestone
Backlog
Development

Successfully merging this pull request may close these issues.
7 participants
@AleksanderBodurri @dgp1130 @alxhub @devversion @josephperrott @thePunderWoman @kirjs
0