dynamic-cdn-webpack-plugin icon indicating copy to clipboard operation
dynamic-cdn-webpack-plugin copied to clipboard

[Propostion] New API to instantiate plugin for diff supported output plugin

Open mastilver opened this issue 7 years ago • 7 comments

I really hate this code: https://github.com/mastilver/dynamic-cdn-webpack-plugin/blob/ba58d17d891a9e0db4d7dd2cca43f7b75ca8baa1/src/index.js#L51-L57

It's working but for the user it can be very surprising and I don't think it can be extended extensively to handle different output plugin (like #31 )

I think a better API would be to expose multiple constructor like (better name ?):

  • DynamicCdnWebpackPlugin
  • DynamicCdnWebpackPLuginForHtmlPlugin
  • DynamicCdnWebpackPluginForAssetsPlugin
  • ...

DynamicCdnWebpackPLuginForHtmlPlugin would extend DynamicCdnWebpackPLugin and would override the output function

@aulisius What do you think? Do you have any other suggestion?

mastilver avatar Sep 01 '18 10:09 mastilver

I like the idea. We can work on the naming but this is interesting. Currently, it is not possible to use DynamicCdn multiple times in the pipeline for different outputs - this new API would enable that.

Personally, not a huge fan of inheritance, is something like this too verbose?

import DynamicCdn, { withAssetsPlugin } from 'dynamic-cdn-webpack-plugin';  

export default {
    context: path.resolve(__dirname, './app'),

    output: {
        publicPath: '',
        path: path.resolve(__dirname, './lib')
    },

    entry: {
        app: './app.js'
    },

    plugins: [
        new AssetsPlugin({
            filename: 'assets.json',
            useCompilerPath: true
        }),
        new DynamicCdn({ output: withAssetsPlugin })
    ]
}

where withAssetsPlugin is basically the function which customizes the output format.

aulisius avatar Sep 02 '18 09:09 aulisius

I'm not sure. I don't think we should let users have access to webpack internals

I understand you are not a fan of inheritance, to me as long as there is only one level, I'm happy

mastilver avatar Sep 05 '18 12:09 mastilver

I concur with the fact that we shouldn't be exposing webpack internals to users. Cool, let's go with the multiple classes approach then. I am okay with inheritance to a certain degree :P

aulisius avatar Sep 05 '18 13:09 aulisius

@aulisius Cool, I will try to work on that this week-end

Any thoughts on the names?

  • DynamicCdnWebpackPLuginForHtmlPlugin
  • HtmlDynamicCdnWebpackPLugin I think I prefer this one
  • HtmlPluginDynamicCdnWebpackPLugin
  • HtmlWebpackDynamicCdnWebpackPLugin

mastilver avatar Sep 14 '18 12:09 mastilver

The names are giving me Java nightmares O_O.

HtmlDynamicCdnWebpackPlugin

I'm cool with this because it's the shortest but like is mentioning Webpack really necessary? The package name already has webpack on it, so we could drop Webpack in the constructor.

aulisius avatar Sep 14 '18 15:09 aulisius

This would be a well received improvement.

moonray avatar Sep 26 '18 12:09 moonray

Needs a little work, but it's a start.

moonray avatar Sep 26 '18 15:09 moonray