-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Add FXIOS-14379 [Performance] Add back equatable for BVC state #31217
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
ih-codes
left a comment
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.
Thanks for fixing the issue with ShareTab Equatable!
| // Setting default page as topsites | ||
| var newTabPageType: NewTabPage = .topSites | ||
| var tabUUID: TabUUID = UUID().uuidString | ||
| // TODO: FXIOS- This can't be nonisolated because it is a var but it is only set once. |
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.
(ticket number)
| import Foundation | ||
|
|
||
| protocol ShareTab: Sendable { | ||
| nonisolated var tabUUID: TabUUID { get } |
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.
Nice, seems a reasonable solution for this weird edge case! 👌
lmarceau
left a comment
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.
+1 on what Isabella said already!
6094f26 to
9debf5e
Compare
📜 Tickets
Jira ticket
💡 Description
Add back equatable to BVC state so that we can do redux diffing. Because ShareTab has a webview and has to be main actor isolated I had to add a nonisolated ID property to equate on.
🎥 Demos
Demo
📝 Checklist