Skip to content

Comments

fix: srcSet with optional descriptor#15905

Merged
patak-dev merged 2 commits intovitejs:mainfrom
murad-khafizov:exposes-bug-srcSet
Feb 20, 2024
Merged

fix: srcSet with optional descriptor#15905
patak-dev merged 2 commits intovitejs:mainfrom
murad-khafizov:exposes-bug-srcSet

Conversation

@murad-khafizov
Copy link
Contributor

@murad-khafizov murad-khafizov commented Feb 14, 2024 •
edited
Loading

There is a bug processing simple <source srcSet='someURL'>, that adds a trailing space at the end of the URL, i.e. it becomes: <source srcSet='someURL '>
That creates a mismatch between SSR-generated HTML and HTML on the client side.

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.

@bolt-new-by-stackblitz
Copy link

bolt-new-by-stackblitz bot commented Feb 14, 2024

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

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 19, 2024

Thanks for the PR. This might be a bug on its own (and thus worth fixing), but I think it would be still nice if you can prepare a reproduction revealing this bug as a Vite user.

For example, do you apply transformIndexHtml to entire SSR render html? It looks like a few issues with this usage have come up recently, but it's not considered to be an officially supported usage for now #15345 (comment) #15662 (comment)

@patak-dev patak-dev changed the title Simple unit-test that exposes a bug in the <source srcSet='...'> processing fix: srcSet with optional descriptor Feb 19, 2024
@patak-dev
Copy link
Member

patak-dev commented Feb 19, 2024

Thanks for the failing test. This exposed a real bug with optional descriptors. Fixed in 87a647f

@murad-khafizov
Copy link
Contributor Author

murad-khafizov commented Feb 19, 2024

For example, do you apply transformIndexHtml to entire SSR render html? It looks like a few issues with this usage have come up recently, but it's not considered to be an officially supported usage for now #15345 (comment) #15662 (comment)

Yes, I apply transformIndexHtml to the entire HTML indeed. Tbh I didn't know it's not recommended, but we used it this way from the very beginning. Anyway thanks, I'll check it.

@patak-dev patak-dev added the p3-minor-bug An edge case that only affects very specific usage (priority) label Feb 19, 2024
@murad-khafizov
Copy link
Contributor Author

murad-khafizov commented Feb 19, 2024

Thanks for the failing test. This exposed a real bug with optional descriptors. Fixed in 87a647f

Thanks a lot for the quick reaction! Looking forward to the new release :)

@patak-dev patak-dev merged commit 81b3bd0 into vitejs:main Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sapphi-red sapphi-red sapphi-red approved these changes

Assignees

No one assigned

Labels

p3-minor-bug An edge case that only affects very specific usage (priority)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@murad-khafizov @hi-ogawa @patak-dev @sapphi-red