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.

Sign up for GitHub

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

Jump to bottom

fix: do not warn (about not being able to bundle non module scripts) when src is an external url #7380

Merged
merged 1 commit into from Mar 19, 2022
Merged

fix: do not warn (about not being able to bundle non module scripts) when src is an external url #7380

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 5 additions & 3 deletions packages/vite/src/node/plugins/html.ts
View file
Open in desktop
Expand Up @@ -297,9 +297,11 @@ export function buildHtmlPlugin(config: ResolvedConfig): Plugin {
someScriptsAreAsync ||= isAsync
someScriptsAreDefer ||= !isAsync
} else if (url && !isPublicFile) {
config.logger.warn(
`<script src="${url}"> in "${publicPath}" can't be bundled without type="module" attribute`
)
if (!isExcludedUrl(url)) {
Copy link
Member

Choose a reason for hiding this comment

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

This has been on my to-fix list for a long while. Thanks for fixing this 😄

My only concern with this is that isExcludedUrl also runs checkPublicFile, which is already covered by isPublicFile. But given this warning don't always happen, I think it's still fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bluwy I thought it was too much to create a new function similar to isExcludedUrl() but without the checkPublicFile() call. Felt more like duplicating code than optimization. However, should you guys want to, I can create a new PR.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's fine the way it is now 👍

config.logger.warn(
`<script src="${url}"> in "${publicPath}" can't be bundled without type="module" attribute`
)
}
} else if (node.children.length) {
const scriptNode = node.children.pop()! as TextNode
const code = scriptNode.content
Expand Down