-
Notifications
You must be signed in to change notification settings - Fork 27k
refactor(devtools): Internal signals prototype #66099
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
base: main
Are you sure you want to change the base?
Conversation
...s/ng-devtools/src/lib/devtools-tabs/directive-explorer/signals-view/signals-tab.component.ts
Show resolved
Hide resolved
packages/forms/src/directives/reactive_directives/abstract_form.directive.ts
Show resolved
Hide resolved
562b23d to
52a7153
Compare
This uses an internal property for prototyping purposes so we don't commit to an API at the moment. fixes angular#65894
52a7153 to
84327b3
Compare
| /** | ||
| * @internal | ||
| */ | ||
| isInternal?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider: Would ɵisInternal improve typing without expanding our public API contract?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@internal actually effectively removes the prop from the public API. (it's not present in the generated d.ts.
| private readonly _submittedReactive = signal(false); | ||
| readonly _submitted = computed( | ||
| () => this._submittedReactive(), | ||
| ...(ngDevMode ? [{isInternal: true} as any] : []), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider: It seems easy to forget the ngDevMode bits here (or misuse it like isInternal: ngDevMode ? true : undefined), especially if we're doing this manually at each call site. We also won't have extensive tests that this is a zero-cost addition in production.
IIRC, the debug name transform wraps an explicit name in an ngDevMode conditional (might be worth double checking that). What if we boxed both debugName and isInternal into a shared debugInfo object and tree shake that value once rather than having to do each one individually?
// Definition
interface DebugInfo {
name?: string;
isInternal?: boolean;
}
declare function signal<T>(initial: T, opts?: {debugInfo?: DebugInfo}): WritableSignal<T>;
// Dev writes:
const foo = signal(0, {
debugInfo: {isInternal: true},
});
// Transformed to (using our existing infra, just tweaked for `debugInfo` instead of `debugName`):
const foo = signal(0, {
...(ngDevMode ? {
debugInfo: {
name: 'foo',
isInternal: true,
},
} : {})
});This way we don't need to think about tree shaking at each call site, we just annotate which signals are internal.
I know we're just experimenting here for now, so I'm fine to come back to this later. I'm mainly calling out that there might be value in a DebugInfo abstraction which allows us to introduce new debug state in the future without having to reinvent the tree shaking solution we need for production.
This uses an internal property for prototyping purposes so we don't commit to an API at the moment.
fixes #65894