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

feat: allow filter overlay message with function #4813

Merged
merged 6 commits into from
May 6, 2023

Conversation

malcolm-kee
Copy link
Contributor

@malcolm-kee malcolm-kee commented Apr 11, 2023

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

For Bugs and Features; did you add new tests?

Motivation / Use-Case

  • Allow filtering of overlay message
  • Fix misleading overlay title when catching runtime error

Resolves

Breaking Changes

Additional Info

@malcolm-kee
Copy link
Contributor Author

Haven't look into test, but based on manual testing on the example it's working. In case anyone want to take over feel free to proceed to fork my implementation and update on top, my time for OSS is quite limited recently. 😢

@codecov
Copy link

codecov bot commented Apr 11, 2023

Codecov Report

Patch coverage: 86.11% and project coverage change: -0.04 ⚠️

Comparison is base (ee748a7) 91.99% compared to head (f7386f6) 91.96%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4813      +/-   ##
==========================================
- Coverage   91.99%   91.96%   -0.04%     
==========================================
  Files          16       16              
  Lines        1673     1704      +31     
  Branches      625      647      +22     
==========================================
+ Hits         1539     1567      +28     
- Misses        123      126       +3     
  Partials       11       11              
Impacted Files Coverage Δ
client-src/index.js 79.86% <76.19%> (-1.25%) ⬇️
lib/Server.js 93.80% <100.00%> (+0.06%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

lib/Server.js Outdated
*/
const encodeOverlaySettings = (setting) =>
typeof setting === "function"
? encodeURIComponent(setting.toString())
Copy link
Contributor Author

@malcolm-kee malcolm-kee Apr 11, 2023

Choose a reason for hiding this comment

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

Usage of encodeURIComponent here is intentional, because without it URLSearchParams will encode white space into + character, which could not be decoded correctly in parseURL in client.

Alternative is to use URLSearchParams in client side, but I think the drawback of that is cannot support old browser.

client-src/index.js Outdated Show resolved Hide resolved
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks good, I have a couple questions

@malcolm-kee
Copy link
Contributor Author

malcolm-kee commented Apr 12, 2023

@alexander-akait slightly out-of-topic, any plan to migrate from puppeteer to playwright or upgrade puppeteer? I can't run test locally which I suspect is because of puppeteer 😢

I realize there is #2843 , maybe will do side quest then

@alexander-akait
Copy link
Member

@malcolm-kee Yeah, it will be great, I think it is not a problem problem, because most of our code is encupsulated also API is almost the same, so feel free to try it I will help

@malcolm-kee malcolm-kee force-pushed the feat/error-filter-fn branch from 4f481cd to f7b1747 Compare May 5, 2023 09:59
@malcolm-kee malcolm-kee force-pushed the feat/error-filter-fn branch from 2dcb797 to 4658fde Compare May 5, 2023 12:51
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks good, I see we don't have tests on it right? is it hard to test?

@malcolm-kee
Copy link
Contributor Author

Will try using addScriptTag: https://pptr.dev/api/puppeteer.page.addscripttag

@alexander-akait
Copy link
Member

Yeah, thank you, feel free to ping me if you will need help

alexander-akait
alexander-akait previously approved these changes May 5, 2023
@alexander-akait
Copy link
Member

Thank you good job, I think we can merge and release it

@malcolm-kee
Copy link
Contributor Author

@alexander-akait I'm adding more tests and realize that the following code will overwrite the overlay settings:

https://github.com/webpack/webpack-dev-server/blob/master/client-src/index.js#L170-L176

What's the use case of supporting that socket message from server?

@alexander-akait
Copy link
Member

@malcolm-kee some tools can't inject your code in webpack bundle process, but they need to enable/disable some features, so we provide ability to do, i.e you can use own websocket server and do it, but I am thinking no one currently use it

@malcolm-kee
Copy link
Contributor Author

Got it. For completeness let me handle that.

@alexander-akait
Copy link
Member

Thank you

@malcolm-kee malcolm-kee marked this pull request as ready for review May 5, 2023 16:27
@malcolm-kee
Copy link
Contributor Author

malcolm-kee commented May 6, 2023

I found bug. Checking... Fixed.

@malcolm-kee
Copy link
Contributor Author

@alexander-akait by the way I also plan to let runtimeErrors catch unhandled promise rejection as well, but probably in next PR after this.

Comment on lines +317 to +319
type: "BUILD_ERROR",
level: "warning",
messages: warnings,
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be BUILD_WARNING?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the event type purpose is to determine how to transition the state:

BUILD_ERROR: {
target: "displayBuildError",
actions: ["setMessages", "showOverlay"],
},
RUNTIME_ERROR: {
target: "displayRuntimeError",
actions: ["setMessages", "showOverlay"],

I don't mind refactor to add event type BUILD_WARNING and remove level property, although it does introduce some duplication in the state machine.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine 👍🏻

Copy link
Member

@snitin315 snitin315 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for working on this 🚀

We will also need to document this on https://webpack.js.org/configuration/dev-server/#overlay

Comment on lines +50 to +54
const overlayFilterFunction = new Function(
"message",
`var callback = ${overlayFilterFunctionString}
return callback(message)`
);
Copy link

Choose a reason for hiding this comment

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

this new Function() breaks devserver for chrome extensions with the following error

Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval' is not an allowed source of script in the following Content Security Policy directive: "script-src 'self'".

it could be fixed by adding 'unsafe-eval' in the manifest but that isn't really an option for us so we're going to roll back for now

Copy link
Member

Choose a reason for hiding this comment

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

@wentokay We can optionally inject this, can you open an issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants