Skip to content

Inline manifest option - #18

Merged
spadgos merged 4 commits into
soundcloud:masterfrom
arundo:inline-manifest-option
Apr 21, 2017
Merged

Inline manifest option#18
spadgos merged 4 commits into
soundcloud:masterfrom
arundo:inline-manifest-option

Conversation

@TuckerCowie

@TuckerCowie TuckerCowie commented Dec 14, 2016

Copy link
Copy Markdown
Contributor

Similar to #15. Adds options.inlineManifest for users who implement html-webpack-plugin;

// webpack.config.js
new ChunkManifestPlugin({
    inlineManifest: true
}),

new HtmlWebpackPlugin({
    template: 'index.ejs'
}),
<!-- index.ejs -->
<body>
    <%= htmlWebpackPlugin.files.webpackManifest %>
</body>

@spadgos

spadgos commented Jan 2, 2017

Copy link
Copy Markdown
Contributor

How does this PR compare to #15 ? Seems to be lot of similarity there

@TuckerCowie

TuckerCowie commented Jan 24, 2017

Copy link
Copy Markdown
Contributor Author

@spadgos, #15 assumes all users of this plugin will use htmlWebpackPlugin. Additionally, it assumes that they will want to inline the manifest. This change, even though the functionality is very similar, is agnostic by allowing the user to specify the configuration via the options object while still defaulting to the current implementation of not inlining.

@pavel06081991

Copy link
Copy Markdown

@diurnalist, could you please review this and merge?

@jouni-kantola

jouni-kantola commented Apr 17, 2017

Copy link
Copy Markdown

FYI as mentioned in PR #15:
inline-chunk-manifest-html-webpack-plugin now uses chunk-manifest-webpack-plugin internally to extract chunks before inlining chunk manifest (default in head).

@spadgos
spadgos merged commit bd40246 into soundcloud:master Apr 21, 2017
@jouni-kantola

jouni-kantola commented Apr 21, 2017

Copy link
Copy Markdown

@spadgos: Why merge a PR making the plugin tightly coupled to html-webpack-plugin, when inline-chunk-manifest-html-webpack-plugin does exactly this? chunk-manifest-webpack-plugin does a fine job doing what it does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants