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.

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

Add more comments in webpack.js #186

Merged
merged 1 commit into from
Nov 19, 2018

Conversation

isaacabraham
Copy link
Member

Added more comments to the webpack.config.js file. I also removed the CONFIG object and embedded it directly into the configuration - I'm not sure if this is better or worse, happy to revert that, but for me as a non-webpack person it means one less object to reason about in the file.

Also renamed the server proxy port to SERVER_PROXY_PORT - otherwise it could be confusing for people using different web servers. Theoretically this is a breaking change so I would like to get this in before v1.

@theimowski
Copy link
Member

/cc @MangelMaxime

I grabbed the CONFIG object from Maxime's suggestions - I think the goal here was to design the webpack config so that the only place a user needs to configure is this CONFIG thing.

Copy link
Member

@theimowski theimowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with added comments and renaming the environment variable name - should make things clearer.
Curious what @MangelMaxime says about the CONFIG object

@MangelMaxime
Copy link
Contributor

The CONFIG object was an idea from Alfonso so people only need to modify this object and don't need to look at the whole webpack.config.js.

But I think, if you are using a template you should not need to modify the webpack.config.js at first. And when you need to modify it, this means you are going to not follow the default configuration and probably going to use advanced features and so learn/understand webpack.

So it's up to you depending on what you prefer. Personnaly, at work I don't have the CONFIG object and only have commonPlugins and babelOptions as variables in order to not declare twice the same things.

// Common/shared variables

var babelOptions = {
    presets: [
        ["@babel/preset-env", {
            "targets": {
                "browsers": ["last 2 versions"]
            },
            "modules": false,
            "useBuiltIns": "usage"
        }],
        "@babel/react"
    ],
    "plugins": [
        "@babel/plugin-proposal-class-properties",
        "@babel/plugin-proposal-object-rest-spread"
    ]
};

var commonPlugins = [
    new HtmlWebpackPlugin({
        filename: './index.html',
        template: './Sources/index.html'
    }),
];

// Usage

module.exports = {
    // ...
    plugins: isProduction ?
        commonPlugins.concat([
            new MiniCssExtractPlugin({
                filename: 'style.[hash].css'
            })
        ])
        : commonPlugins.concat([
            new webpack.HotModuleReplacementPlugin(),
            new webpack.NamedModulesPlugin()
        ]),
    // ...
    module: {
        rules: [
            {
                test: /\.fs(x|proj)?$/,
                use: {
                    loader: "fable-loader",
                    options: {
                        babel: babelOptions
                    }
                }
            },
            {
                test: /\.js$/,
                exclude: /node_modules/,
                use: {
                    loader: 'babel-loader',
                    options: babelOptions
                },
            },
        ]
    }
    // ...
};

@theimowski
Copy link
Member

Fair enough, let's inline that CONFIG object then. Merging - thanks @isaacabraham and @MangelMaxime

@theimowski theimowski merged commit b10b9b2 into SAFE-Stack:master Nov 19, 2018
@isaacabraham isaacabraham deleted the webpack-comments branch November 19, 2018 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants