feat: open editor when clicking error on overlay #4587
Conversation
malcolm-kee
requested review from
alexander-akait,
hiroppy,
snitin315 and
anshumanv
as code owners
Codecov ReportBase: 91.96% // Head: 91.99% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #4587 +/- ##
==========================================
+ Coverage 91.96% 91.99% +0.02%
==========================================
Files 16 16
Lines 1655 1661 +6
Branches 622 623 +1
==========================================
+ Hits 1522 1528 +6
Misses 122 122
Partials 11 11
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
lib/launchEditor.js
Outdated
| } | ||
| } | ||
|
|
||
| module.exports = launchEditor; |
There was a problem hiding this comment.
Maybe there is a package for this? Also can we add unit tests for this?
There was a problem hiding this comment.
I've installed launch-editor package, which apparently is extracted from react-dev-utils. It's also used by Vite as well.
lib/Server.js
Outdated
| @@ -2033,6 +2034,25 @@ class Server { | |||
| } | |||
| ); | |||
|
|
|||
| /** @type {import("express").Application} */ | |||
| (app).get( | |||
There was a problem hiding this comment.
Let's add e2e test for this
There was a problem hiding this comment.
Yup, it's added!
malcolm-kee
force-pushed
the
feat/overlay-open-editor
branch
from
72f8612
to
44611e8
Compare
|
|
||
| /** | ||
| * @param {Parameters<import('puppeteer').Page['emulate']>[0]} config | ||
| * @returns {Promise<RunBrowserResult>} |
There was a problem hiding this comment.
Additional typing to improve intellisense while writing tests.
|
|
||
| if (typeof fileName === "string") { | ||
| // @ts-ignore | ||
| const launchEditor = require("launch-editor"); |
There was a problem hiding this comment.
require lazily to make mock possible.
| @@ -203,6 +203,16 @@ function show(type, messages, trustedTypesPolicyName) { | |||
| typeElement.innerText = header; | |||
| applyStyle(typeElement, msgTypeStyle); | |||
|
|
|||
| if (message.moduleIdentifier) { | |||
| applyStyle(typeElement, { cursor: "pointer" }); | |||
| typeElement.dataset.canOpen = true; | |||
There was a problem hiding this comment.
this is hook for e2e test. If there is other preferred convention, let me know!
malcolm-kee
force-pushed
the
feat/overlay-open-editor
branch
from
44611e8
to
24bd59e
Compare
There was a problem hiding this comment.
Sorry for delay, it looks good, @snitin315 Can you review this too?
|
@malcolm-kee Thank you, do we will have more PRs before release? |
|
@alexander-akait I plan to have one more enhancement for reporting runtime error, but I foreseen that will probably take time as we probably want to allow configuration (e.g. |
|
Thanks for answer, no rush, I will wait, thank you |
|
@alexander-akait Right now I'm a bit stuck with how to differentiate between error thrown by webpack vs application code, do you have any suggestion? Let me know if this should be discussed in another channel! Meanwhile I'll continue try to understand how it was done in |
|
@malcolm-kee Feel free to send a PR how you see to improve it and we will continue dicussion in the PR, thank you |
|
@malcolm-kee This function does not seem to work, how should I verify this function |
|
@nanianlisao can you raise an issue with a reproduction? We have test for this so it's apparently working for us. |
|
@malcolm-kee Sorry about that, my bad. I thought we could do without the error-overlay-webpack-plugin plugin, but it looks like we're not there yet. We can only catch errors that are passed through the WebSocket for now. |
For Bugs and Features; did you add new tests?
Motivation / Use-Case
This PR make webpack-dev-server open editor when user clicks on the error message on overlay.
Part of #3689.
Breaking Changes
Additional Info
"/webpack-dev-server/open-editor"is added on the dev server which the client would calls usingfetch.moduleIdentifierfrom thestats.errorsis the file name, but I not sure if that assumption is correct.launch-editorto open editor (it is based onreact-dev-utilsof Create React App, and is being used by Vite as well).moduleIdentifier, but I assume that is not a dealbreaker.