Updates to webpack.config.js and webpack.tests.config.js (discussion)

[wtorricos]

Hey guys, recently I recreated the template from scratch https://github.com/tico321/SAFEFromScratch

And there are a few changes I would like to bring to the webpack config files so they are more aligned with js best practices, but before I create a PR please let me know your thoughts about the following changes:

• Use const instead of var to declare variables.
• Update the webpack.tests.config.js file to have only what's necessary for testing as there is no need to have production building logic in there.
• Add some tasks that we can run with npm run and invoke them from Fake (see this package.json for an example), so instead of running:

  "client", dotnet "fable watch -o output -s --run webpack-dev-server --config ../../webpack.tests.config.js" clientTestsPath

  we would run:

  "client", dotnet "fable watch -o output -s --run npm start:tests --prefix ../.." clientTestsPath

• Identify the environment based on the value coming from process.env.NODE_ENV if you reviewed the package.json above this is done by passing the --node-env flag.
• Move the bulma, react and react-dom to dependencies instead of devDependencies in the package.json file

[olivercoad]

Nice work with SAFEFromScratch! That's incredible, really well detailed 👍

I agree with all those points except maybe the second one. What if you want to run tests on a production build? I think I remember at one point there were issues that should've shown up in the tests but didn't because the test config didn't use bable when the main config did. Or some kind of similar situation, I can't remember exactly what the issue was. Ideally, making the test config as similar as possible will help to reduce the likelihood of encountering that kind of heisenbug.

[wtorricos]

I can't argue with that argument hehe I'll create a PR for the rest of the changes as soon as the PR to update dependencies is merged.

[theimowski]

Wow that's what you call reverse engineering - really impressive @tico321! As long as the webpack changes conform to the industry standards than cool :+1: As for keeping main and test webpack config files - I recall @Zaid-Ajaj had some good reasons to do that. On the other hand it makes maintenance harder - every change needs to be performed in both files. Wondering if there's a way to share some logic between these two config files?

[wtorricos]

Thank you @theimowski ! I'm glad you liked it. Regarding the changes, Oliver made me realize we should definitely include a prod configuration for webpack.tests.config.js. as it can be very beneficial to run the tests on prod mode in a CI pipeline. And yes there is a way to share logic between them, please take a look at bonus 3 in my tutorial https://github.com/tico321/SAFEFromScratch I added it once I realized we want it. And yes that would be part of the changes I want to bring.

[olivercoad]

Using the CONFIG like you have looks really nice, it makes it really clear what the differences are between the main and test configs.

On the topic of webpack config, I've always wondered if it would be better to get the env by exporting a function.

https://webpack.js.org/configuration/configuration-types/#exporting-a-function https://webpack.js.org/api/cli/#environment-options

Something like this:

TEST_CONFIG = {
   ...
}

MAIN_CONFIG = {
   ...
}

module.exports = (env, argv) => {
   const CONFIG = env.tests ? TEST_CONFIG : MAIN_CONFIG;
   const isProduction = argv.mode === 'production';
   const environment = isProduction ? 'production' : 'development';
   console.log('Bundling for ' + environment + (env.tests ? ' tests' : '') + '...');
   return {
      ...
   };
}

And used like

webpack --mode development --env tests

[wtorricos]

I didn't know you could do that, and I think I like it better than exporting an object. The only problem I see is that most documentation and tutorials export an object, so I'm not sure if it will be the best.

[wtorricos]

After playing with it, returning a function does look cleaner. I think I'm going to send the PR with that, and let's see if I get more feedback.