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: injects scripts into html files without any JS in mpa mode #17968

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

proc07
Copy link
Contributor

@proc07 proc07 commented Aug 29, 2024

fix issues #17943
I'm not confident that my solution is sound, and if there are no issues, I plan to add unit tests.

The result after running locally:
截屏2024-08-29 09 48 17

Copy link

stackblitz bot commented Aug 29, 2024

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

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

It would be great to have tests for this. You can use playground/legacy for it. One thing I wonder too if it works correctly if the HTML has <script type="module" but points to a public file, in that case we shouldn't be removing the script.

@@ -508,6 +508,12 @@ function viteLegacyPlugin(options: Options = {}): Plugin[] {
transformIndexHtml(html, { chunk }) {
if (config.build.ssr) return
if (!chunk) return
if (!chunk.moduleIds.some((moduleId) => moduleId.endsWith('.js'))) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is the right check, probably chunk.imports.length === 0 is more correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But in the following case, chunk.imports is an empty array, and index.html is the needs to include a polyfill script.
截屏2024-09-02 15 21 28

Copy link
Member

Choose a reason for hiding this comment

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

Hmm strange, I was testing using playground/legacy/index.html locally and was able to see the imports have the JS chunks, not sure what's going on here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the config file
playground/legacy/vite.config-multiple-input.js

Copy link
Contributor Author

@proc07 proc07 Sep 2, 2024

Choose a reason for hiding this comment

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

I created the /playground/legacy/mpa/index.html and only-element.html files, you can see the image below, the imports array is empty. Would it be better to change the judgment condition to !chunk.moduleIds.some((moduleId) => moduleId.endsWith('.js')) && chunk.imports.length === 0
截屏2024-09-02 22 56 47

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.

2 participants