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

[DevTools] Add open in editor #22649

Merged
merged 10 commits into from Nov 3, 2021
Merged

[DevTools] Add open in editor #22649

merged 10 commits into from Nov 3, 2021

Conversation

ezzak
Copy link
Contributor

@ezzak ezzak commented Oct 28, 2021

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

Screen Shot 2021-10-31 at 10 22 10 PM

Copy link

@Sarkar44 Sarkar44 left a comment

Choose a reason for hiding this comment

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

review change

Copy link
Contributor

@jstejada jstejada left a 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

@@ -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';
Copy link
Contributor

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

@bvaughn
Copy link
Contributor

bvaughn commented Oct 29, 2021

Give me a few minutes. I'll take a look this morning.

@bvaughn
Copy link
Contributor

bvaughn commented Oct 29, 2021

@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}"`,
Copy link
Contributor

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
Copy link
Contributor

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

Screen Shot 2021-10-29 at 10 11 32 AM

Copy link
Contributor

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.

Copy link
Contributor

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:

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:

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>
);

Copy link
Contributor

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));
Copy link
Contributor

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.

Copy link
Contributor

@bvaughn bvaughn Oct 29, 2021

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?

@ezzak
Copy link
Contributor Author

ezzak commented Nov 1, 2021

Updated to make the url configurable:

  • Assumption: URL should have the two tokens "{path}" and "{line}" which we fill in, we kinda need these to be tokens as different apps do things differently.

VSCode Urls: https://code.visualstudio.com/docs/editor/command-line#_opening-vs-code-with-urls
Sublime Urls: https://github.com/asuth/subl-handler

@ezzak ezzak requested a review from bvaughn November 1, 2021 18:22
Copy link
Contributor

@bvaughn bvaughn left a 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

@bvaughn
Copy link
Contributor

bvaughn commented Nov 1, 2021

Also confirmed that it works in the extension:

open-in-editor-2

@jstejada
Copy link
Contributor

jstejada commented Nov 1, 2021

so good! excited for this to land!

Copy link
Contributor

@bvaughn bvaughn left a 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:

  1. Practically everyone at FB uses VSCode and on-demand.
  2. VS Code is popular externally but there are other editors too.

@ezzak
Copy link
Contributor Author

ezzak commented Nov 2, 2021

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.

Dino1981 referenced this pull request Nov 2, 2021
useId is the updated version of this API.
Copy link
Contributor

@bvaughn bvaughn left a 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.

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
@bvaughn bvaughn merged commit 255221c into facebook:main Nov 3, 2021
@ezzak ezzak changed the title [DevTools] Add open in editor for fb [DevTools] Add open in editor Nov 3, 2021
KamranAsif pushed a commit to KamranAsif/react that referenced this pull request Nov 4, 2021
Co-authored-by: Brian Vaughn <bvaughn@fb.com>
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
Co-authored-by: Brian Vaughn <bvaughn@fb.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants