-
-
Notifications
You must be signed in to change notification settings - Fork 292
Introduce overlayStyleMode option to manage overlay styling behavior
#470
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
base: main
Are you sure you want to change the base?
Conversation
| "express": "^4.17.1", | ||
| "file-loader": "^6.2.0", | ||
| "morgan": "^1.10.0", | ||
| "style-loader": "^4.0.0", |
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.
Sorry, we can't use it here, because it will be this module for many people, we need to search another solution for this problem
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.
Thanks for the review. I implemented the current solution by considering all possible scenarios. By default, I intentionally did not auto-import the CSS file so that no loader is required out of the box. When CSP-compatible styling is needed, the CSS file must be imported by user manually, which naturally requires a CSS loader to be configured.
The style-loader and file-loader here are used only for the csp example (not the package itself):
https://github.com/ertgl/webpack-hot-middleware/blob/fee93b3c934579dde5b54d49770dd8598870c965/example/webpack.config.csp.js#L34
With this approach, users can integrate the package into their own setups using their preferred loaders, or without any loaders at all. If I misunderstood your point, I'm happy to revisit it.
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.
To be honest, I'm more interested in why inline styles aren't allowed in development mode... but I am open to any solution, I just want to understand that this problem is really worth these workarounds instead of allowing inline styles in development mode
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.
Some projects, especially security-sensitive ones (e.g. legal-related), enforce CSP consistently across all environments to prevent issues early. Some developers also aim to keep their development environment as close to production as possible, while some teams work directly on staging servers over SSH to share results with other departments. So the CSP aspect is just one part of that consistency; the broader goal is maintaining full control over quality and velocity, regardless of the environment. As a small benefit, by disabling inline styles in development, it becomes a written rule for the project, and the browser acts like a nice real-time linter, helping catch issues before tests even run.
Currently, the package interrupts such disciplines. Allowing inline styles for a single development package can also open the door for other third-party packages to accidentally inline styles, giving developer the impression that "it worked, so it's finished." I see this as an opt-in feature containing the fix, instead of a workaround.
|
|
||
| ### CSP-compatible Styling | ||
|
|
||
| Since inline styles are disallowed, styles must be imported from a CSS file. To demonstrate this, the example uses `file-loader` and `style-loader` by setting the `injectType` option to `linkTag`. |
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've added a description of why we install file-loader and style-loader in the example package.
Summary
overlayStyleModeoption with two possible values:inline(default) andheadless.client-overlay.cssfile containing the default styles.README.md, describeoverlayStyleMode.cspandheadless-stylingscenarios.What kind of change does this PR introduce?
Did you add tests for your changes?
CSP scenario:
Headless-styling scenario:
Does this PR introduce a breaking change?
No.
If relevant, what needs to be documented once your changes are merged or what have you already documented?
Documented in the
README.md:overlayStyleModeoption.inline.webpack-hot-middleware/client-overlay.csscan be imported to enable the default styles.overlayStylesoption requiresoverlayStyleModeto beinline.Not documented:
CHANGELOG.md