fix(html): Template files should only include entry chunks
Rollup Plugin Name: html
This PR contains:
- [x] bugfix
- [ ] feature
- [ ] refactor
- [ ] documentation
- [ ] other
Are tests included?
- [x] yes (bugfixes and features will not be merged without tests)
- [ ] no
Breaking Changes?
- [ ] yes (breaking changes will not be merged unless absolutely necessary)
- [x] no
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers: #634
Description
I ran into a regression in #634 where non-entry scripts are injected into the default html template. This can cause unexpected behavior especially if those scripts have side-effects. This PR fixes the file filter and also adds a guard in case rollup emitted any javascript asset files.
Thanks for the PR! You're on the right path, but just off of it. I think the idea is good for default behavior as well, and while it is breaking, it'll make default behavior for others easier to understand when the build gets a little bigger. So good call.
Where you want to make the change though, is here: https://github.com/rollup/plugins/blob/a54d2a111458141a8298f8d7fb4505da63950ada/packages/html/src/index.ts#L41-L46
@shellscape Sounds good- actually, I looked at that approach too but went the other direction because I thought the behavior change was a mistake in https://github.com/rollup/plugins/commit/a1b2fc3e6df5b1630acd8d5e3dbeea96ceef5800. I'll update this PR 👍
Wait, doesn't removing the filter from getFiles allow the template function to get the full stack of files? My intention in https://github.com/rollup/plugins/pull/688/commits/233fe7ccbb7f685c7c4ef7b79f13033e4e8a49a0 was to allow all files to flow to the template function, moving the isEntry filter to the default template implementation only.
About the filter itself, I am using some assumptions about Rollup's OutputAsset|OutputChunk type. Assuming that .type must equal 'chunk' or 'asset', the filter logic from a1b2fc3 already seems to be a no-op:
(file) =>
file.type === 'chunk' ||
(typeof file.type === 'string' ? file.type === 'asset' : file.isAsset)
But maybe I am missing something- is that assumption invalid? Let me know, I am ready to tweak the PR as needed, just need more clarification...