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

Unblock LSC - Patch 2 - Fix initializer of instance members that reference identifiers declared in the constructor #6901

Merged
merged 4 commits into from
Aug 27, 2024

Conversation

rileyajones
Copy link
Contributor

Motivation for features / changes

There is an ongoing LSC cl/652971829 which changes how instance methods are set. The change is blocked because of our split codebase. This PR is intended as a replacement for go/tbpr/6883

@rileyajones rileyajones requested a review from arcra August 16, 2024 18:39
@rileyajones rileyajones changed the title Unblock LSC - Patch 2 Unblock LSC - Patch 2 - Fix initializer of instance members that reference identifiers declared in the constructor Aug 16, 2024
readonly stackFramesForDisplay$;

constructor(private readonly store: Store<State>) {
this.codeLocationType$ = this.store.pipe(
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this one in the before snapshot. Was this change intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a strange case where the code is actually duplicated. I removed the other copy.

@arcra
Copy link
Member

arcra commented Aug 19, 2024

About the failures, I can see some error messages related to being "[unable to] bind to 'cardObserver' since it isn't a known property of [some components]", which (I'm speculating, because I'm not super familiar with FE/Angular) causes other is not a known element error messages that are somewhat misleading.

That cardObserver input was changed in #6900, so (again, speculating) this issue might only be related to some of these observables that were recently changed to be initialized in the constructor, and which are used as inputs for other components. It's possible that adding a type annotation could fix it (?).

Then again... CI passed for that previous change, and I'm not sure if something in the changes on this PR make it so it only breaks with these new changes, so all of this might be a red herring (sorry if that's the case)... but just adding some thoughts here, for later.

image

@arcra
Copy link
Member

arcra commented Aug 19, 2024

About the failures, I can see some error messages related to being "[unable to] bind to 'cardObserver' since it isn't a known property of [some components]", which (I'm speculating, because I'm not super familiar with FE/Angular) causes other is not a known element error messages that are somewhat misleading.

That cardObserver input was changed in #6900, so (again, speculating) this issue might only be related to some of these observables that were recently changed to be initialized in the constructor, and which are used as inputs for other components. It's possible that adding a type annotation could fix it (?).

Then again... CI passed for that previous change, and I'm not sure if something in the changes on this PR make it so it only breaks with these new changes, so all of this might be a red herring (sorry if that's the case)... but just adding some thoughts here, for later.

image

Riley mentioned this might be just a typing issue, but unrelated to the test breakages, which would make sense since it was not a blocker for the previous PR to be merged.

@rileyajones rileyajones merged commit b9ab242 into tensorflow:master Aug 27, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants