Skip to content

Unify webpack configuration with webpack-config-template#204

Merged
theimowski merged 10 commits intoSAFE-Stack:masterfrom
theimowski:unify_webpack
Jan 7, 2019
Merged

Unify webpack configuration with webpack-config-template#204
theimowski merged 10 commits intoSAFE-Stack:masterfrom
theimowski:unify_webpack

Conversation

@theimowski
Copy link
Copy Markdown
Member

Idea is to take webpack configuration from https://github.com/fable-compiler/webpack-config-template , adjust a couple of configuration values and include in the template, instead of maintaining a separate different webpack conf

@theimowski
Copy link
Copy Markdown
Member Author

/cc @alfonsogarciacaro @isaacabraham @MangelMaxime want to review?

Related: #195 #194 #152

@theimowski theimowski changed the title [WIP] Unify webpack Unify webpack configuration with webpack-config-template Dec 20, 2018
Comment thread Content/src/Client/webpack.config.js Outdated
// In development, bundle styles together with the code so they can also
// trigger hot reloads. In production, put them in a separate CSS file.
entry: isProduction ? {
app: [resolve(CONFIG.fsharpEntry), resolve(CONFIG.cssEntry)]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can simplify the entries by removing resolve.

Webpack expect an absolute path only for the output.path node.

Copy link
Copy Markdown
Member Author

@theimowski theimowski Dec 20, 2018

Choose a reason for hiding this comment

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

The webpack.config.js is now downloaded from webpack-config-template repository. Actually I might consider gitignoring this file to prevent confusion

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Ah ok, so this why the build.fsx is modifying it to add api things.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Exactly, as @alfonsogarciacaro suggested, we should stick to single source of truth, which I'm happy to do

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Yes i have no problem with that :)

I just review this PR as asked. We can wait to see if Alfonso wants to modify the original file with my comments or no I guess.

Comment thread Content/src/Client/webpack.config.js Outdated
};

function resolve(filePath) {
return path.isAbsolute(filePath) ? filePath : path.join(__dirname, filePath);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You can simplify by using:

function resolve(filePath) {
    return path.join(__dirname, filePath)
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Same as above - the file comes from webpack-config-template

This was referenced Jan 2, 2019
@theimowski theimowski merged commit b5c2fa4 into SAFE-Stack:master Jan 7, 2019
@theimowski theimowski deleted the unify_webpack branch January 7, 2019 13:07
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.

2 participants