-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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: convert overlay template to DOM #15852
Conversation
Run & review this pull request in StackBlitz Codeflow. |
IIUC if a user is using |
If you don't already have hyperscript in the codebase, I'll rewrite it with DOM. NBD. |
Ok. I've converted this over to DOM. I added a small pair of helper functions to make reading the creation of the actual element easier. |
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!
Co-authored-by: 翠 / green <green@sapphi.red>
Co-authored-by: 翠 / green <green@sapphi.red>
Co-authored-by: 翠 / green <green@sapphi.red>
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.
💚
For me, I'm not a big fan of needing to tweak Vite in order to support CSP. I think it should be additive and not affect how we write code. Personally I would prefer if the changes are done via Vite plugins at the meantime if this change is required. (You can That said, I've talked with @sapphi-red and @patak-dev about this, and I'm okay if we go with the first commit instead that uses But ultimately, I'd prefer a better CSP story altogether before we support this. #14653 is one where IIRC an option might be needed, and the overlay could perhaps leverage it. |
Hey all, what's the best way forward here? I've got one approval and one saying no |
@Snugug the PR is now in the team board. We'll discuss it live on the next meeting so we can give you a definite answer about how to move forward. I would also like to see this applied, and both approaches are fine to me to be honest. |
Sgtm thank you! |
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 discussed this proposal in the last Vite team meeting and we decided to move forward with it. We'd like to keep the current approach using h()
to have a better out-of-the-box story. Thanks for your patience and work on this PR and the templates one @Snugug. We'll be merging this one as part of Vite 5.2.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
/ecosystem-ci run |
📝 Ran ecosystem CI on
|
This seems to have broken this import format inside web workers as fetch('https://esm.run/svelte@next/compiler.cjs').then(res => {
res.text().then(source => {
const blob = new Blob([source], { type: 'text/javascript' });
import(/* @vite-ignore */URL.createObjectURL(blob)).then((_svelte) => {
svelte = _svelte;
});
})
}) |
@juho would you create an issue with a minimal reproduction so we can properly track it? |
Problem was introduced in vite 5.2.0 with vitejs/vite#15852
This breaks web worker, see #16308 |
@patak-dev there is a reproduction and issue tracking for this regression in #16277 (comment) |
Description
Adds Trusted Types support to the Error overlay.
Resolves #15850
Resolves #10553
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).