plugins icon indicating copy to clipboard operation
plugins copied to clipboard

fix(html): Template files should only include entry chunks

Open ianpurvis opened this issue 5 years ago • 3 comments

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.

ianpurvis avatar Dec 04 '20 19:12 ianpurvis

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 avatar Dec 06 '20 02:12 shellscape

@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 👍

ianpurvis avatar Dec 06 '20 17:12 ianpurvis

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

ianpurvis avatar Dec 07 '20 16:12 ianpurvis