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

feat(plugin-webpack): customize HtmlWebpackPlugin options #2969

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(plugin-webpack): customize HtmlWebpackPlugin options #2969

Show file tree
Hide file tree
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion packages/plugin/webpack/src/Config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { Configuration as RawWebpackConfiguration } from 'webpack';
import { Configuration as RawWebpackConfiguration, WebpackPluginInstance } from 'webpack';
import WebpackDevServer from 'webpack-dev-server';
import { ConfigurationFactory as WebpackConfigurationFactory } from './WebpackConfig';
import HtmlWebpackPlugin from 'html-webpack-plugin';

type ExtraHtmlPluginOptions = Omit<HtmlWebpackPlugin.Options, 'title' | 'template' | 'filename' | 'chunks'>;

export interface WebpackPluginEntryPoint {
/**
Expand Down Expand Up @@ -48,6 +51,20 @@ export interface WebpackPluginEntryPoint {
* for all entries.
*/
nodeIntegration?: boolean;
/**
* Custom options to merge into the configuration passed to `HtmlWebpackPlugin`.
*/
htmlOptions?: Partial<ExtraHtmlPluginOptions>;
/**
* Plugins to include before `HtmlWebpackPlugin`; typically, HTML plugin add-ons will
* need to be placed here.
*/
htmlPlugins?: WebpackPluginInstance[];
Comment on lines +58 to +62
Copy link
Member

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

Copy link
Author

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.

/**
* Additional options to merge into the Webpack `output` configuration for this entry-
* point.
*/
output?: object;
Comment on lines +63 to +67
Copy link
Member

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?

Copy link
Author

@sgammon sgammon Oct 16, 2022 •
edited
Loading

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?

}

export interface WebpackPreloadEntryPoint {
Expand Down
3 changes: 3 additions & 0 deletions packages/plugin/webpack/src/WebpackConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ export default class WebpackConfigGenerator {
devtool: this.rendererSourceMapOption,
mode: this.mode,
output: {
...(entryPoint.output || {}),
path: path.resolve(this.webpackDir, 'renderer'),
filename: '[name]/index.js',
globalObject: 'self',
Expand All @@ -205,9 +206,11 @@ export default class WebpackConfigGenerator {
__filename: false,
},
plugins: [
...(entryPoint.htmlPlugins || []),
...(entryPoint.html
? [
new HtmlWebpackPlugin({
...(entryPoint.htmlOptions || {}),
Copy link
Author

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

title: entryPoint.name,
template: entryPoint.html,
filename: `${entryPoint.name}/index.html`,
Expand Down
31 changes: 31 additions & 0 deletions packages/plugin/webpack/test/WebpackConfig_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,37 @@ describe('WebpackConfigGenerator', () => {
await generator.getRendererConfig(config.renderer.entryPoints);
expect(getInvokedCounter()).to.equal(2);
});

it('honors custom entrypoint output options', async () => {
const { MyWebpackConfigGenerator } = makeSubclass();

const config = {
mainConfig: () => ({
entry: 'main.js',
...sampleWebpackConfig,
}),
renderer: {
config: { ...sampleWebpackConfig },
entryPoints: [
{
name: 'main',
js: 'rendererScript.js',
output: {
crossorigin: 'anonymous',
Copy link
Author

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

},
},
],
},
} as WebpackPluginConfig;

const generator = new MyWebpackConfigGenerator(config, mockProjectDir, false, 3000);

const rendererConfig = await generator.getRendererConfig(config.renderer.entryPoints);
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const outputSettings = rendererConfig[0].output as any;
expect(outputSettings).not.to.be.undefined;
expect(outputSettings['crossorigin']).to.equal('anonymous');
});
});
});
});