Skip to content
This repository has been archived by the owner on Sep 7, 2020. It is now read-only.

Better default webpack config #1210

Closed
drazik opened this issue Dec 1, 2017 · 14 comments
Closed

Better default webpack config #1210

drazik opened this issue Dec 1, 2017 · 14 comments

Comments

@drazik
Copy link
Contributor

drazik commented Dec 1, 2017

Hello,

I just ran npm run build and saw that the resulting CSS file is not minified. So I looked at @phenomic/plugin-bundler-webpack default webpack config file and effectively, css-loader's minimize option is not used at all.

Is this intentional or is it really missing ? If it is missing, I can propose a PR to fix it :)

@MoOx
Copy link
Owner

MoOx commented Dec 1, 2017

In fact the default config is probably not good enough.
We should replace this one somehow by grabbing the one from CRA (via react-scripts https://github.com/facebookincubator/create-react-app/tree/9af042263f7c6e66b4c1402d2f6fc81250c6ce97/packages/react-scripts/config)
See #1190 and #1200 for more info.

@MoOx
Copy link
Owner

MoOx commented Dec 1, 2017

PR welcome as it should not be that hard to get the config from
react-script (via import/require) and merge it using spread operator and the things we need if relevant (hot loading probably?)

@MoOx MoOx changed the title Missing CSS minification Better default webpack config Dec 1, 2017
@drazik
Copy link
Contributor Author

drazik commented Dec 1, 2017

Just to be sure that I understand what you want to achieve : the idea is to add react-scripts to @phenomic/plugin-bundler-webpack dependencies, then import its webpack configs (depending on the NODE_ENV and PHENOMIC_ENV) and maybe override some stuff ?

@MoOx
Copy link
Owner

MoOx commented Dec 1, 2017

Exactly! Can feel weird in the first place, but for now it's for us the simplest way to rely on a good (and up to date) common config widely used (and so accepted)!
This might be revisited later, but for now, it's for us a good way to not spend time on creating a config to make everyone happy.

@drazik
Copy link
Contributor Author

drazik commented Dec 1, 2017

OK I will try to work on that :) Thank you for the tips

@MoOx
Copy link
Owner

MoOx commented Dec 1, 2017

If you need anything, just ask!

@drazik
Copy link
Contributor Author

drazik commented Dec 1, 2017

I see that the project uses Lerna. To add a dependency on a particular package, do you use lerna add module --scope=package or directly yarn add module in the directory of the package ?

@MoOx
Copy link
Owner

MoOx commented Dec 1, 2017

To be honest I don't use any of this solution. I add the dep by hand in the package.json of the relevant package and then I do yarn at the root of the repo :D

@drazik
Copy link
Contributor Author

drazik commented Dec 1, 2017

Ok so I guess you don't really care how I install react-scripts

@MoOx
Copy link
Owner

MoOx commented Dec 1, 2017

As you wish. Should be the same at the end anyway.

@drazik
Copy link
Contributor Author

drazik commented Dec 4, 2017

I looked at it, began to work on a solution (using webpack-merge) and I have some questions :

  • how can I override this loader configuration specifically ? I don't want to touch others, but for this one I need to use @phenomic/babel-preset and react-hot-loader/babel plugin. The only way I think about to override it is to override all the module.rules array, but it is dumb because we would lost react-scripts configuration here
  • I have the same issue with the ExtractTextPlugin configuration : we want to disable it when process.env.PHENOMIC_ENV !== "static", but I can't figure how to override react-scripts's configuration for it. Same as above, I can override the whole configuration, but will lose the defaults in that case

I think that react-scripts webpack configuration is pretty good, but rely on create-react-app custom stuff that is not necessary in phenomic. Maybe we could write our custom configuration that would be heavily inspired by react-scripts one, but not directly import it and override it. What do you think about it ? From my point of view, it doesn't seem to be more work than override everything.

@MoOx
Copy link
Owner

MoOx commented Dec 4, 2017

I guess we should not use wepack-merge and do the merge by hand. Should not be hard imo.
We can probably loop on rules and use test.toString() to see the regex for the js(x) loader (but we should merge with eslint-loader as it's used (imo) in a safe maner.
For ExtractTextPlugin it's easy, loop + instanceof (as we have a "new" ^^). I had do did this in previous version of phenomic.

I understand your point, but my idea is to rely on the package so we can (in theory) easily upgrade. And say "we rely on this". People will trust this more than a copy/adapted :)

@drazik
Copy link
Contributor Author

drazik commented Dec 4, 2017

I see. Instead of trying to deep merge react-scripts config + current phenomic config, I should handpick what's interesting in react-scripts config (particularly loaders).

@MoOx
Copy link
Owner

MoOx commented Feb 1, 2019

Closing due to lack of interest. My idea is really to have a minimal config that people should extends by themselves if they want to. Might revisit later.

@MoOx MoOx closed this as completed Feb 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants