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(server): file request response content type contains utf-8 #14453

Closed
wants to merge 4 commits into from

Conversation

btea
Copy link
Collaborator

@btea btea commented Sep 24, 2023

Description

Additional context


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.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • 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).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@stackblitz
Copy link

stackblitz bot commented Sep 24, 2023

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

@bluwy
Copy link
Member

bluwy commented Sep 25, 2023

There's some discussion about this at #14142 (comment). I'm still open to this possibility, but I'm not sure if it's entirely safe for dev to assume utf-8 always.

@btea
Copy link
Collaborator Author

btea commented Sep 25, 2023

When I opened the project using Edge, I encountered the following warning message, so I created this PR.

image

@sapphi-red
Copy link
Member

It seems the warning is not shown if it's localhost. That's why I didn't find it in past.

I wonder if it's better to leave it so that users would add <meta charset="utf-8"> in case the server is not configured to send the charset.


While I think we can add it to HTML, I think it's incorrect to set charset=utf-8 to binary files. Also I think we don't need to add to JS files and JSON files.

I'm not sure about CSS files. If we set it, it will override the @charset value.
https://developer.mozilla.org/en-US/docs/Web/CSS/@charset#:~:text=The%20value%20of,is%20UTF%2D8
Vite assumes the file to be encoded as UTF-8, but raw files (e.g. files in public) can use other encodings.

@bluwy
Copy link
Member

bluwy commented Nov 14, 2023

Also since #14756, charset=utf-8 is not set for HTML by default in preview. Dev also doesn't set it as before. I think it's fair for users to use <meta charset="utf-8"> manually, or add a middleware that auto-sets it.

@btea
Copy link
Collaborator Author

btea commented Nov 19, 2023

@bluwy @sapphi-red Thank you for your detailed comments. According to the For JS files, ESM files must be encoded as UTF-8 and is always processed as UTF-8 mentioned by @sapphi-red , so maybe I just need to add chartset=utf-8 here to determine whether it is a js file?

@bluwy
Copy link
Member

bluwy commented Nov 20, 2023

I still don't quite see why Edge is flagging this an issue. From what I understand, we don't have to set charset=utf8 for JS, that's only a recommendation.

The screenshot links to these:

And it doesn't provide a strong reason for JS files.

@sapphi-red
Copy link
Member

No, that would set charset=utf-8 to classic scripts as well. (classic scripts doesn't need to be UTF-8.)
ESM files will always be processed as UTF-8 so there's no reason to specify by the mime type.

@btea
Copy link
Collaborator Author

btea commented Nov 20, 2023

Thank you for your detailed reply. Based on your views, we should maintain the status quo and I will close this PR.

@btea btea closed this Nov 20, 2023
@btea btea deleted the feat/content-type-set-utf-8 branch November 20, 2023 11:20
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.

3 participants