-
-
Notifications
You must be signed in to change notification settings - Fork 520
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
feat(plugin-webpack): customize HtmlWebpackPlugin
options
#2969
base: main
Are you sure you want to change the base?
Conversation
0ba5296
to
1b8ae31
Compare
i see what seems like a flaky test locally (unrelated), and i'm unclear about how i might reach into the |
Codecov Report
@@ Coverage Diff @@
## main #2969 +/- ##
==========================================
+ Coverage 73.80% 73.84% +0.03%
==========================================
Files 66 66
Lines 2142 2145 +3
Branches 424 427 +3
==========================================
+ Hits 1581 1584 +3
Misses 356 356
Partials 205 205
Continue to review full report at Codecov.
|
HtmlWebpackPlugin
options
/** | ||
* Additional options to merge into the Webpack `output` configuration for this entry- | ||
* point. | ||
*/ | ||
output?: object; |
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'm not clear on the use case for this option, it directly contradicts the goals of forge which is to manage these disk assets (E.g. we don't allow configuration of out/dir for other tools) specifically so we're always in control of and know where things are on disk. i.e. this will break the environment variables that we provide for folks to get the path to the webpack generated files
Can you clarify exactly what you want to pass in here and why?
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.
yes -- in some cases, plugin adopters need to adjust the output
parameter in concert with a hook into the build. for example, the SRI plugin needs {output: {crossorigin: 'anonymous'}}
, but only for targets which invoke HtmlWebpackPlugin
.
i did what i could to avoid this object, but i don't see an alternative -- any ideas? specifically, for applying options to the output
stanza of only the HTML targets?
/** | ||
* Plugins to include before `HtmlWebpackPlugin`; typically, HTML plugin add-ons will | ||
* need to be placed here. | ||
*/ | ||
htmlPlugins?: WebpackPluginInstance[]; |
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.
Instead of passing in a random list of plugins here, could we just change our implementation to ensure the config is extended with user plugins appearing first in the list instead of always adding ours before? That would avoid this extra configuration option completely
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 intent of isolating this as its own property instead of adjusting the plugin flow is three-fold:
1) less invasive.
i suspect downstream projects rely on the order of plugins, so i am doing my best not to adjust the existing execution order in any way that might affect how existing projects work.
2) no way to include only where HTML targets exist.
for example, say you have several targets, a few with exotic renderers or something, and then just one target with HtmlWebpackPlugin
. if you use the global plugins
property, it will apply to all entries instead of just the applicable invocation of HtmlWebpackPlugin
.
3) semantic meaning.
because these are the only plugins that must run before HtmlWebpackPlugin
, this should keep the plugin flow easy to reason about for future changes.
these are just the thoughts behind this approach. i am totally open to thoughts on better typing or alternate approaches.
1b8ae31
to
07f8368
Compare
This changeset adds options to entrypoints to support customized operation of the `HtmlWebpackPlugin`. See below for full changeset. Changes enclosed: - Add properties for `output`, `htmlPlugins`, and `htmlOptions` to `WebpackPluginEntryPoint` - Use new options from `Config.ts`, by merging them into their expected places - Add tests to cover new options Fixes and closes electron#2968.
07f8368
to
6565ca7
Compare
...(entryPoint.html | ||
? [ | ||
new HtmlWebpackPlugin({ | ||
...(entryPoint.htmlOptions || {}), |
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.
reworked to use spread operator
name: 'main', | ||
js: 'rendererScript.js', | ||
output: { | ||
crossorigin: 'anonymous', |
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.
test showing sample use of output
i should also say, @MarshallOfSound, i have some background material on the linked issue, #2698. the alternatives listed there were considered but imagined to be suboptimal. thank you for all your hard work on this library, and for the lightning fast review -- it makes working with Electron so easy and smooth! |
accidental close (fat thumb) |
What is blocking this PR from beeing merged? It would be very useful for us 🙏🏼 |
Hi, |
Summarize your changes:
This changeset adds options to entrypoints to support customized operation of the
HtmlWebpackPlugin
. See below for full changeset.Changes enclosed:
output
,htmlPlugins
, andhtmlOptions
toWebpackPluginEntryPoint
Config.ts
, by merging them into their expected placesFixes and closes #2968.