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: convert overlay template to DOM #15852

Merged
merged 9 commits into from Mar 12, 2024
Merged

Conversation

Snugug
Copy link
Contributor

@Snugug Snugug commented Feb 8, 2024

Description

Adds Trusted Types support to the Error overlay.

Resolves #15850
Resolves #10553


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines, especially the Pull Request Guidelines.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Feb 8, 2024

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@sapphi-red
Copy link
Member

IIUC if a user is using Content-Security-Policy: trusted-types 'a-policy-name-that-they-use';, then the user has to add 'error-overlay' to that. I wonder if it's better to construct the element with document.createElement (or hyperscript?) so that they don't need to add 'error-overlay'.

@Snugug
Copy link
Contributor Author

Snugug commented Feb 9, 2024

If you don't already have hyperscript in the codebase, I'll rewrite it with DOM. NBD.

@Snugug Snugug changed the title feat: add trusted types support to error overlay feat: convert overlay template to DOM Feb 9, 2024
@Snugug
Copy link
Contributor Author

Snugug commented Feb 9, 2024

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.

Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Thanks!

packages/vite/src/client/overlay.ts Outdated Show resolved Hide resolved
packages/vite/src/client/overlay.ts Outdated Show resolved Hide resolved
packages/vite/src/client/overlay.ts Outdated Show resolved Hide resolved
Snugug and others added 4 commits February 11, 2024 06:33
Co-authored-by: 翠 / green <green@sapphi.red>
Co-authored-by: 翠 / green <green@sapphi.red>
Co-authored-by: 翠 / green <green@sapphi.red>
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

💚

@bluwy
Copy link
Member

bluwy commented Feb 13, 2024

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 transform() overlay.js)

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 trustedTypes to support this, and we have documentation for how to incorporate this.

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.

@Snugug
Copy link
Contributor Author

Snugug commented Feb 15, 2024

Hey all, what's the best way forward here? I've got one approval and one saying no

@patak-dev
Copy link
Member

@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.

@Snugug
Copy link
Contributor Author

Snugug commented Feb 15, 2024

Sgtm thank you!

@patak-dev patak-dev added the p2-to-be-discussed Enhancement under consideration (priority) label Feb 21, 2024
Copy link
Member

@patak-dev patak-dev left a 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.

@patak-dev

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@patak-dev
Copy link
Member

/ecosystem-ci run

@patak-dev patak-dev added this to the 5.2 milestone Feb 21, 2024
@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 6f2d871: Open

suite result latest scheduled
analogjs success success
astro success success
histoire success success
ladle success success
laravel success success
marko success success
nuxt success success
nx failure failure
previewjs success success
qwik success success
rakkas success success
sveltekit success success
unocss success success
vike success success
vite-plugin-pwa success success
vite-plugin-react success success
vite-plugin-react-pages success success
vite-plugin-react-swc success success
vite-plugin-svelte success success
vite-plugin-vue success success
vite-setup-catalogue success success
vitepress success success
vitest success success

@bluwy bluwy merged commit dd49505 into vitejs:main Mar 12, 2024
6 of 10 checks passed
@juho
Copy link

juho commented Mar 24, 2024

This seems to have broken this import format inside web workers as document is not available:

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;
        });
    })
})

@patak-dev
Copy link
Member

@juho would you create an issue with a minimal reproduction so we can properly track it?

carlobeltrame added a commit to carlobeltrame/ecamp3 that referenced this pull request Mar 26, 2024
@Mister-Hope
Copy link

This breaks web worker, see #16308

@jlarmstrongiv
Copy link

@patak-dev there is a reproduction and issue tracking for this regression in #16277 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-to-be-discussed Enhancement under consideration (priority)
Projects
Archived in project
7 participants