-
Notifications
You must be signed in to change notification settings - Fork 22
Create FB-only react_devtools_placeholder panel #36
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
Conversation
6e35be3 to
94d11da
Compare
| const ReactDevToolsPlaceholder = await loadReactDevToolsPlaceholderModule(); | ||
| return ReactDevToolsPlaceholder.ReactDevToolsPlaceholder.ReactDevToolsPlaceholderImpl.instance(); | ||
| }, | ||
| experiment: Root.Runtime.ExperimentName.REACT_DEVTOOLS_PLACEHOLDER_PANEL_ENABLE, |
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.
Can we actually hide this panel / placeholder if ENABLE_REACT_DEVTOOLS_PANEL flag is enabled? This was added in eba41f4
Maybe change logic a bit in a way that if flag is not enabled -> render placeholder. Otherwise, we may end up in a state, when flag is enabled and we render 2 panels: one is placholder and the other is experimental React DevTools panel.
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.
if flag is not enabled
I'd love to, but can't find any pattern to invert an experiment check in this way in the codebase 😐. What we will have instead is one configuration point where it should be clear we'll disable one experiment over the other.
Root.Runtime.experiments.enableExperimentsByDefault([
Root.Runtime.ExperimentName.REACT_NATIVE_SPECIFIC_UI,
Root.Runtime.ExperimentName.REACT_DEVTOOLS_PLACEHOLDER_PANEL_ENABLE,
]);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.
Bit surprised CDT doesn't have the convention of "disabled experiment". In that case, sounds like it would it be simpler to
- remove the existing experiment field from RDT view registration
- modify the existing RDT view registration to load this placeholder based on the existing experiment value.
Depending on where we check the experiment, we can mark the RDT experiment registration as "restart required" too.
That way we have a single deterministic experiment and always have the RDT panel visible (goal of this PR).
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.
We could do this, but I don't think the juice is worth the squeeze, as we'll remove this placeholder soon enough. Main blocker is the RDT panel hasn't landed yet.
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.
Discussed offline with @hoxyq — will remove my experiment flag and enable by default. Ruslan will build on top of this to use this panel as the RDT panel.
94d11da to
fe5f2a7
Compare
hoxyq
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.
Discussed offline, I will update my changes in #17 to change paths / ids for this panel, and which will gate the contents of this panel (stub / real RDT view)
| >npx react-devtools</code | ||
| > | ||
| </div> | ||
| <a |
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.
I don't think this fburl quite makes sense considering that we launch CDT in a Chrome guest profile.
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.
Summary
Note
Depends on #34 ✅ and #35 ✅. Please review the latest stacked commit.
Adds an FB-only
react_devtools_placeholderpanel and enables in thern_fuseboxentry point. This panel is intended to be temporary, until we internally launch the React DevTools feature.Test plan
Upstreaming plan
devtools-frontendrepo. I've reviewed the contribution guide.