Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
chore: add @rollup/plugin-html #57
chore: add @rollup/plugin-html #57
Changes from all commits
8c96ec1
9773508
abf179f
5670a79
7dc69ec
20598ef
36bb3a2
6ef057f
d757c3c
83b7e72
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function should ONLY get entry point files (isEntry: true) and ignore the rest. Reason:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disagree here. Various assets can be added to a bundle's files via plugins, and we should pick those up. CSS is the major example here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function puts all files .js files into an array and throws away all meta information. It is virtually impossible to distiguish the auto-generated chunks from the entry points. This makes the plugin rather useless for code-splitting IMO. Either you provide some meta information, or you ignore the non-entry chunks. I would be very confused if a CSS plugin was broken if you ignored the auto-generated chunks, as they are still imported by the main JavaScript. And maybe this even breaks CSS plugins in so far as maybe their chunks are meant to be loaded dynamically together with their content while you make them load up-front instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes the case for providing the bundle metadata to the
template
option function for complex builds that the default template doesn't suit.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be sufficient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With regard to my initial comment, maybe I mis-stated what I mean: This function should of course return all assets, but with regard to chunks, it should only return entry chunks, as all non-entry chunks (read: .js files) will be loaded automatically by the entry chunks, no additional work necessary. But additionally having the bundle available to the template still would not hurt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name will be ignored as a
fileName
is provided.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted. We need to update the Rollup docs as this is a copy/paste/modify from them. I'll see about doing that this week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is written in the first paragraph after the code block here: https://rollupjs.org/guide/en/#thisemitfileemittedfile-emittedchunk--emittedasset--string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did read that, but with the code sample above that having both and both being optional, I had assumed it would be prudent to set
name
in the event that something was inspecting it down the line. Some extra verbiage there could be useful to point out there's no need to set the other property.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. It does not hurt adding the name, it is just ignored.