-
Notifications
You must be signed in to change notification settings - Fork 47.3k
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
[DevTools] Add open in editor #22649
Conversation
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.
review change
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.
this is amazing, thank you!! overall lgtm! do you thing you could add a screen recording of what this looks like to the test plan? cc @bvaughn
packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js
Show resolved
Hide resolved
@@ -20,6 +20,7 @@ import {ElementTypeSuspense} from 'react-devtools-shared/src/types'; | |||
import CannotSuspendWarningMessage from './CannotSuspendWarningMessage'; | |||
import InspectedElementView from './InspectedElementView'; | |||
import {InspectedElementContext} from './InspectedElementContext'; | |||
import {isInternalFacebookBuild} from 'react-devtools-feature-flags'; |
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 wonder if we should add a feature flag for this feature, i think probably we don't need to, but checking to see if @bvaughn has thoughts
Give me a few minutes. I'll take a look this morning. |
@Sarkar44 If you have specific suggestions or ideas, they're welcome, but please don't leave vague comments on PRs. (I've noticed a few comments in the past 24 hours like this so I'm mentioning it explicitly.) |
@@ -92,6 +93,7 @@ module.exports = { | |||
__TEST__: NODE_ENV === 'test', | |||
'process.env.DEVTOOLS_PACKAGE': `"react-devtools-extensions"`, | |||
'process.env.DEVTOOLS_VERSION': `"${DEVTOOLS_VERSION}"`, | |||
'process.env.EDITOR_URL': `"${EDITOR_URL}"`, |
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.
How do we feel about making this a user configurable setting that could maybe also accept a default initial value via process.env
?
That way VS Code users outside of Facebook could also benefit from this.
@@ -268,3 +272,7 @@ const PATH_VIEW_DOM = ` | |||
const PATH_VIEW_SOURCE = ` | |||
M9.4 16.6L4.8 12l4.6-4.6L8 6l-6 6 6 6 1.4-1.4zm5.2 0l4.6-4.6-4.6-4.6L16 6l6 6-6 6-1.4-1.4z | |||
`; | |||
|
|||
const PATH_VS_CODE = ` | |||
M 17.488281 2.394531 L 9.058594 10.148438 L 4.34375 6.597656 L 2.394531 7.730469 L 7.042969 12 L 2.394531 16.269531 L 4.34375 17.40625 L 9.058594 13.851562 L 17.488281 21.605469 L 21.605469 19.605469 L 21.605469 4.394531 Z M 17.488281 7.496094 L 17.488281 16.503906 L 11.511719 12 Z M 17.488281 7.496094 |
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.
VS Code is pretty popular, and easily recognizable, but I wonder if we should use a more generic icon for this?
Maybe something like:
M7 5h10v2h2V3c0-1.1-.9-1.99-2-1.99L7 1c-1.1 0-2 .9-2 2v4h2V5zm8.41 11.59L20 12l-4.59-4.59L14 8.83 17.17 12 14 15.17l1.41 1.42zM10 15.17L6.83 12 10 8.83 8.59 7.41 4 12l4.59 4.59L10 15.17zM17 19H7v-2H5v4c0 1.1.9 2 2 2h10c1.1 0 2-.9 2-2v-4h-2v2z
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.
Sorry to interrupt you. I want to ask how is this gigantic string get converted to an icon. I never saw something like this until now. what it is called tell me the name and I will learn about it online.
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.
It's an SVG path.
Check out these components to see how it's used:
Icon
:
react/packages/react-devtools-shared/src/devtools/views/Icon.js
Lines 88 to 98 in 9c8161b
return ( | |
<svg | |
xmlns="http://www.w3.org/2000/svg" | |
className={`${styles.Icon} ${className}`} | |
width="24" | |
height="24" | |
viewBox="0 0 24 24"> | |
<path d="M0 0h24v24H0z" fill="none" /> | |
<path fill="currentColor" d={pathData} /> | |
</svg> | |
); |
ButtonIcon
:
react/packages/react-devtools-shared/src/devtools/views/ButtonIcon.js
Lines 140 to 154 in 9c8161b
return ( | |
<svg | |
xmlns="http://www.w3.org/2000/svg" | |
className={`${styles.ButtonIcon} ${className}`} | |
width="24" | |
height="24" | |
viewBox="0 0 24 24"> | |
<path d="M0 0h24v24H0z" fill="none" /> | |
{typeof pathData === 'string' ? ( | |
<path fill="currentColor" d={pathData} /> | |
) : ( | |
pathData | |
)} | |
</svg> | |
); |
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 the info. I got to know something new.
const url = new URL(editorURL); | ||
url.searchParams.set('project', 'facebook-www'); | ||
url.searchParams.append('paths[0]', source.fileName); | ||
url.searchParams.append('lines[0]', String(source.lineNumber)); |
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.
Obviously this part is pretty Facebook-specific, so maybe my comment to leave this open for non-Facebook users is too optimistic for initial launch. Still, it seems like maybe there's something we could eventually do here.
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.
What this URL ends up doing on the server side is just returning a redirect to the browser:
fb-vscode://nuclide.core/open-arc?project=facebook-www&path=...&line=...
VS Code supports a similar URL handler for the public releases, aka
vscode://file/c:/myProject/
So maybe the thing to do here would be for us to switch our behavior – whether we use the simple scheme like this or the Facebook specific URL query based on the isInternalFacebookBuild
flag?
Updated to make the url configurable:
VSCode Urls: https://code.visualstudio.com/docs/editor/command-line#_opening-vs-code-with-urls |
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.
Tested it with an OSS build and CRA:
open-in-editor.mp4
so good! excited for this to land! |
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.
This is, overall, great!
One requested change:
For the internal build, let's actually pre-fill in the editor URL value (rather than just a placeholder).
For the OSS version, we can leave the placeholder as the default (bc not everyone uses VSCode).
In other words...
OSS build:
- No default value.
- Placeholder text for vscode (like you currently have).
FB build (or whenever DevTools is built with process.env.EDITOR_URL):
- Default value that equals process.env.EDITOR_URL.
- Placeholder value equal to process.env.EDITOR_URL if default value is erased.
My thinking:
- Practically everyone at FB uses VSCode and on-demand.
- VS Code is popular externally but there are other editors too.
Addressed comments: Updated behavior: Without env variable: process.env.EDITOR_URL defaulted to null. Setting is disabled by default. If user goes to settings they can see the vscode default url as a placeholder and use it if they wish to. If user types anything in this the setting gets enabled. With env variable: process.env.EDITOR_URL is a string. Setting is enabled by default and uses this url. If user goes to settings they can see it pre-filed in. If they delete it they can disable the setting or they can override it to a desired value. |
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 seeing this through! I'm going to push a few small tweaks, then we can land.
packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js
Outdated
Show resolved
Hide resolved
packages/react-devtools-shared/src/devtools/views/Settings/ComponentsSettings.js
Outdated
Show resolved
Hide resolved
packages/react-devtools-shared/src/devtools/views/Settings/ComponentsSettings.js
Outdated
Show resolved
Hide resolved
1. Removed an unnecessary console.log 2. Added extra space/padding around new editor URL setting 3. Removed 'restart needed' label and updated setting to listen for change event
Co-authored-by: Brian Vaughn <bvaughn@fb.com>
Co-authored-by: Brian Vaughn <bvaughn@fb.com>
Summary
Add ability to view the current component in your code editor.
Test Plan
Open with vscode (default behavior):
openincode.mov
Open in sublime (customized behavior):
devtoolssublime.mov
Icon updated after these videos were captured