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

Webpack improvements #491

Merged
merged 8 commits into from Jan 14, 2022
Merged

Webpack improvements #491

merged 8 commits into from Jan 14, 2022

Conversation

wtorricos
Copy link

@wtorricos wtorricos commented Dec 8, 2021

I want to propose the following changes to webpack in order to align with common practices in the JS world. Here is the list of changes:

  • Use const instead of var when possible.
  • Split devDependencies and dependencies in the package.json file.
  • Use script commands from Fake instead of calling webpack directly.
  • Make config and test config share the same logic to avoid having to update both configurations every time.
  • Identify the environment and mode through environment variables and arguments passed to webpack.

Some additional changes:

  • I updated the copy-webpack-plugin to the latest version (10.0.0)
  • I updated [hash] to [contenthash] for the mini-css-extract-plugin and for the js output since [hash] has been deprecated.

I'll be happy to hear your feedback on this proposal, so please let me know if anything can be improved.

closes #488

Walter Torricos added 4 commits December 8, 2021 08:42
- Change [hash] to [contenthash] for the files generated as hash is deprecated.
- Move react to 'dependencies' instead of 'devDependencies' in the package.json file.
- Update Copy plugin to latest version.
Get the environment from process.env.NODE_ENV
: commonPlugins,
resolve: {
// See https://github.com/fable-compiler/Fable/issues/1490
symlinks: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Does anyone know if this is still needed when not using fable-loader webpack loader?

@olivercoad
Copy link
Contributor

Has anyone tried writing their Webpack config in F# yet? 🤪

@wtorricos
Copy link
Author

Has anyone tried writing their Webpack config in F# yet? 🤪

Not yet, but could be interesting. I'm thinking that using anonymous records could be the easiest thing to do.

@theimowski
Copy link
Member

Looks good 👍 @Zaid-Ajaj @MangelMaxime can you have a look please?

@MangelMaxime
Copy link
Contributor

All the stuff listed in @tico321 comments seems goods to me.

One tips, that I like to use now days for the plugins array is to use .Filter(Boolean) as I find it easier to read and add new plugins depending on the target:

        plugins:
            [
                new HtmlWebpackPlugin({
                    filename: "./index.html",
                    template: "./src/index.html"
                }),
                new MiniCssExtractPlugin(),
                isProduction && myProductionOnlyPlugin() // This plugin will only be added for production
            ].filter(Boolean),

But it depends on what people find here to read between that and the concat.

About the symlink comment, I suppose it is not required anymore has Fable doesn't retrieve the file via webpack anymore.

@olivercoad
Copy link
Contributor

Yep, I think I like that. That way you don't have to scroll up to find commonPlugins half a screen earlier. Not quite as clean as F# (implicit) yields though 😅. It does use some javascript idiosyncrasies that not everyone might be familiar with, but I think with a comment there it's probably fine.

@wtorricos
Copy link
Author

As it looks like we no longer need the symlink I just removed it.
I also played a little bit with the filter option that @MangelMaxime suggested and I think it does look cleaner, so I added that modification as well (Note that I also updated the comments and placed them above the plugins).
Any feedback is welcome, so please let me know if it looks awkward and I can rollback to use a common plugins variable.

@wtorricos
Copy link
Author

Is anything preventing the PR from being merged? Please let me know if I missed something

@theimowski theimowski merged commit c04aca6 into SAFE-Stack:master Jan 14, 2022
@theimowski
Copy link
Member

Sorry, didn't have time to get to this up till now.
This looks awesome - many thanks!
There are a few things we still need to wrap up before releasing new template version

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.

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