-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: support Trusted Types for client overlay #4404
Conversation
|
Thanks for PR, can you accept CLA? |
client-src/index.js
Outdated
@@ -15,7 +15,7 @@ import createSocketURL from "./utils/createSocketURL.js"; | |||
* @property {boolean} hot | |||
* @property {boolean} liveReload | |||
* @property {boolean} progress | |||
* @property {boolean | { warnings?: boolean, errors?: boolean }} overlay | |||
* @property {boolean | { warnings?: boolean, errors?: boolean, policyName?: string }} overlay |
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 am wondering whether the policyName
should be renamed to more verbose trustedTypesPolicyName
. WDYT?
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 don't think we will have the policyName
option here, but I think make sense to rename it to trustedTypesPolicyName
, to avoid misleading
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.
Fixed in c63489b
client-src/overlay.js
Outdated
@@ -23,10 +23,26 @@ let iframeContainerElement; | |||
let containerElement; | |||
/** @type {Array<(element: HTMLDivElement) => void>} */ | |||
let onLoadQueue = []; | |||
/** @type {any} */ |
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.
There are TS type definitions located in https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/trusted-types
However, I am not sure if it is worth typing so I've added any
for now.
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 think we need install them in dev depedencies, because when types is not found it is any
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.
Fixed 68d3974
*/ | ||
function createContainer(policyName) { | ||
// Enable Trusted Types if they are available in the current browser. | ||
if (window.trustedTypes) { |
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.
We could optionally check whether the policyName
is not null
. However, I think using trustedTypes whenever they are available is better because:
- It is still noop and backwards compatible change for applications which do not enforce Trusted Types
- This enables Trusted Types integration without the need to change anything in webpack configuration (which can be hidden, such as in CRA). This means that the only change the application needs to do is whitelist the default policy name.
types/lib/Server.d.ts
Outdated
@@ -1610,6 +1610,10 @@ declare class Server { | |||
negatedDescription: string; | |||
}; | |||
}; | |||
policyName: { |
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 am not sure why there are so many unrelated changes in this autogenerated d.ts
file. Especially, since I've added just one field.
Please, LMK if I have something wrong in my setup.
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 is bug in ts
, ignore it
05c2242
to
fc7962a
Compare
I also see that there needs to be a documentation update in https://webpack.js.org/configuration/dev-server/#overlay, specifically this file https://github.com/1ilsang/webpack.js.org/blob/master/src/content/configuration/dev-server.mdx#overlay I guess that is only updated after there is a new release? Also, please LMK if there are is anything in this PR I've missed :) |
We will open issue in our docs after merge, dont worry
Yes, I will do release |
devServer: { | ||
client: { | ||
overlay: { | ||
policyName: "overlay-policy", |
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.
Consider using webpack#dev-overlay
or something like this to feature the recommended naming scheme.
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.
Fixed in 8ef1a48
@Siegrift frinedly ping |
Sorry, I got a bit busy with work stuff this week. I'll hopefully find some time tomorrow or over the weekend. Thanks for the ping :) |
Bih thanks, feel free to ping me |
function createContainer(trustedTypesPolicyName) { | ||
// Enable Trusted Types if they are available in the current browser. | ||
if (window.trustedTypes) { | ||
overlayTrustedTypesPolicy = window.trustedTypes.createPolicy( |
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.
In case there is an error creating a Trusted Types policy the overlay is not shown at all.
A different approach would be to capture the exception outside this function (same place we define overlayTrustedTypesPolicy
) and show an overlay without using innerHTML
showing that the policy creation failed.
However I think having an error in the console is sufficient, since this is a development only feature so we can assume the console is opened anyways.
I've fixed the comments and rebased on current |
@Siegrift Hello, sorry for delay, can you update snapshots? |
26d875c
to
d8da5ca
Compare
Sorry for not running the tests locally first :/ |
@Siegrift help output changed because you added |
Thanks, I am having problems passing the tests locally. When I run Is there something I am doing wrong? |
Are you using macOS? |
Yes, the m1 chip. |
Yeah, help output snapshots are not quite well on macOS, known bug in nodejs it strips the actual output on stdout. |
I'll try to fix the tests on my Linux machine later today. I don't think I'll get anywhere by testing this on macOS :D |
I want to merge it and fix tests in the next PR, because we need to do release today due security problems in portfinder, big thanks for PR |
No problem, sorry for taking it too long :/ Good luck with the release. |
For Bugs and Features; did you add new tests?
Yes. LMK if more are needed.
Motivation / Use-Case
Addresses #4400
Breaking Changes
No breaking changes.
This change is noop for applications which do not enforce Trusted Types. Applications which do enforce Trusted Types and use webpack-dev-server can now whitelist
webpack-dev-server#overlay
policy to make error overlay work. Optionally, users can choose a different name viapolicyName
attribute (but maybe this could change to more verbosetrustedTypesPolicyName
?).Additional Info
See #4400