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

fix(html): empty script #6057

Merged
merged 4 commits into from Dec 12, 2021
Merged

fix(html): empty script #6057

merged 4 commits into from Dec 12, 2021

Conversation

poyoho
Copy link
Member

@poyoho poyoho commented Dec 10, 2021

Description

fix: #6008

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

Shinigami92
Shinigami92 previously approved these changes Dec 11, 2021
bluwy
bluwy previously approved these changes Dec 11, 2021
@patak-dev
Copy link
Member

I don't know if this is the right fix, we should check why there isn't a string for that cached proxy. If an empty string is inserted in the cache (instead of undefined as I assume it is now), then we don't need to throw an error

@poyoho
Copy link
Member Author

poyoho commented Dec 11, 2021

I don't know if this is the right fix, we should check why there isn't a string for that cached proxy. If an empty string is inserted in the cache (instead of undefined as I assume it is now), then we don't need to throw an error

@patak-dev thanks comment.

why there isn't a string for that cached proxy

so we need to find why isn't a string for that cached proxy case to fix? avoid using if to catch edge case.
I guess import "index.html?html-proxy&index=1.js" is used manually 😀.

@patak-dev
Copy link
Member

I think the problem needs to be here

const contents = node.children

@poyoho poyoho dismissed stale reviews from bluwy and Shinigami92 via dba0c69 December 12, 2021 01:49
@poyoho
Copy link
Member Author

poyoho commented Dec 12, 2021

I think script children must be a TextNode, so the cache must be string.

@patak-dev
Copy link
Member

You are right, the initial implementation was fine. Sorry for the noise, reverted it for you. Let's merge 👍🏼

@patak-dev patak-dev merged commit 1487223 into vitejs:main Dec 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty script in html prevents loading
5 participants