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

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

Open
mastilver opened this Issue Sep 1, 2018 · 5 comments

Comments

Projects
None yet
2 participants
@mastilver
Owner

mastilver commented Sep 1, 2018

I really hate this code:

const isUsingHtmlWebpackPlugin = HtmlWebpackPlugin != null && compiler.options.plugins.some(x => x instanceof HtmlWebpackPlugin);
if (isUsingHtmlWebpackPlugin) {
this.applyHtmlWebpackPlugin(compiler);
} else {
this.applyWebpackCore(compiler);
}

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?

@aulisius

This comment has been minimized.

Show comment
Hide comment
@aulisius

aulisius Sep 2, 2018

Collaborator

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.

Collaborator

aulisius commented Sep 2, 2018

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.

@mastilver

This comment has been minimized.

Show comment
Hide comment
@mastilver

mastilver Sep 5, 2018

Owner

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

Owner

mastilver commented Sep 5, 2018

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

@aulisius

This comment has been minimized.

Show comment
Hide comment
@aulisius

aulisius Sep 5, 2018

Collaborator

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

Collaborator

aulisius commented Sep 5, 2018

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

@mastilver

This comment has been minimized.

Show comment
Hide comment
@mastilver

mastilver Sep 14, 2018

Owner

@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
Owner

mastilver commented Sep 14, 2018

@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
@aulisius

This comment has been minimized.

Show comment
Hide comment
@aulisius

aulisius Sep 14, 2018

Collaborator

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.

Collaborator

aulisius commented Sep 14, 2018

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.

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