-
Notifications
You must be signed in to change notification settings - Fork 372
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
WIP: Allow re-symbolication to use the WebChannel, take 2 #3656
base: main
Are you sure you want to change the base?
WIP: Allow re-symbolication to use the WebChannel, take 2 #3656
Conversation
With this change, retrieveProfileForRawUrl now always waits for the profile to be received, and calls loadProfile with initialLoad = true in all cases.
This is a somewhat unclean intermediate state. It introduces no behavior changes and passes tests, but it's more code. Future patches will remove the code duplication.
…le and into the callers. Similar to the previous commit, this also some code duplication, while making no changes to behavior. Once createBrowserConnection has been moved all the way to the outside, there will be only one call to it, and all parts of the code will use the same BrowserConnection instance.
…ction call further outside.
This moves the call to createBrowserConnection all the way to the outside. It also removes the user agent override: We will now only wait for the WebChannel if we're running in a browser with a Firefox userAgent. Fixes firefox-devtools#3638. I've kept the tests mostly as-is. I'm planning to simplify them in future commits.
…olicate Profile button.
Codecov Report
@@ Coverage Diff @@
## main #3656 +/- ##
==========================================
- Coverage 88.90% 88.86% -0.05%
==========================================
Files 258 259 +1
Lines 21498 21581 +83
Branches 5494 5519 +25
==========================================
+ Hits 19112 19177 +65
- Misses 2212 2229 +17
- Partials 174 175 +1
Continue to review full report at Codecov.
|
I gave this more thought and I think this might actually be the way to go. I can't think of any trouble it could cause. It would definitely solve the problem: Connected Components can simply grab the browserConnection(Status) object from the redux state and short-circuit the downwards property propagation; it's what they're designed to do. |
This PR supersedes #3504 and passes tests. However, the implementation is rather ugly. The
browserConnection
prop needs to be passed all the way down the following chain:<Root>
-><AppViewRouter>
-> (<ZipFileViewer>
->)<ProfileViewer>
-><MenuButtons>
-><MetaInfoPanel>
.I'd love to hear suggestions for alternative approaches! I've considered the following:
onResymbolicateButtonClick
.React.createContext()
, or put it in a global. It would make the testing situation easier, though. I also don't know how it interacts with connected components.This PR makes the following workflow work:
┆Issue is synchronized with this Jira Task