-
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
PoC: support launch editor feature for selected component #20821
Conversation
Comparing: 1a74726...fce4828 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
3f83e57
to
8d86de4
Compare
launchEditorEndpoint: string, | ||
fileName: string, | ||
lineNumber: string, | ||
|}; |
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.
Question: What do the | do around the enums? Never seen that before?
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.
Just do same as other type definintion around the file, maybe it's @flow
's grammer, I am not sure. I don't have enough experience about @flow
to explain it. There is a detail explain -> https://flow.org/en/docs/types/unions/
packages/react-devtools-shared/src/devtools/views/ButtonIcon.js
Outdated
Show resolved
Hide resolved
packages/react-devtools-shared/src/devtools/views/Settings/GeneralSettings.js
Outdated
Show resolved
Hide resolved
packages/react-devtools-shared/src/devtools/views/Settings/GeneralSettings.js
Outdated
Show resolved
Hide resolved
packages/react-devtools-shared/src/devtools/views/Settings/SettingsContext.js
Show resolved
Hide resolved
8d86de4
to
404b4af
Compare
404b4af
to
fce4828
Compare
The react/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js Line 260 in c5cfa71
|
And there is no need to think too much about the endpoint URI, let the server-side logic think about it |
I almost forgot about this pull request, thanks for activation @Airkro . I will resolve the merge conflicts later. |
lineNumber, | ||
}: LaunchEditorParams) => { | ||
fetch( | ||
`/${launchEditorEndpoint}?fileName=${fileName}&lineNumber=${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.
This looks pretty Unix/Mac specific. Am I reading it wrong?
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 just a url pointing to the server, so I think it should be fine
Related PR: #22649 |
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated. |
Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you! |
Summary
support launch editor feature
Original Feature Request #20435
When you inspected a react component, you can click the
launch editor
icon to jump to your editor and navigate to the source code original file position.supoort set custom
launchEditorEndpoint
Why we need this ? Because different launch editor server middleware use different endpoint.
create-react-app
user must use__open-stack-frame-in-editor
as endpoint cause these hard code.launch-editor-middleware
user often use__open-in-editor
as endpoint.Both middleware have many user around world (and exist many custom middleware actually).
Issues need to fix
source
path is absolute(e.g./Users/iChenLei/Documents/cra/reactapp/src/App.js:24
).create-react-app/react-dev-utils
to support absolute path.https://github.com/facebook/create-react-app/blob/master/packages/react-dev-utils/launchEditor.js#L287-L290
+ const filePath = path.relative(process.cwd(), fileName)
launch editor
feature, Thanks forfacebook/react
official.common toast
ui component to show message about set custom endpoint success ? I think feedback is necessary.@bvaughn Sir, I need your code review and advices, thanks.
Test Plan
will add necessary unit test later
Circle CI Artifacts
Every one can download this build artifacts to test
launch editor
, but you must modilfy yourlaunch editor
middleware to supportabsolute file path
otherwise you can't launch editor success. I will create a pull request tocreate-react-app/react-dev-utils
to support absolute file path. Welcome any suggestion !// for example // ./node_modules/react-dev-utils/launchEditor.js function launchEditor(fileName, lineNumber, colNumber) { + fileName = path.relative(process.cwd(), fileName) if (!fs.existsSync(fileName)) { return; }