Get under control webpack config files. #691

AlexKVal · 2015-05-18T11:04:52Z

Scripts compiling webpack config files are somewhat convoluted now
and they are getting harder to maintain and understand
example

It all leads to confusions like this and this

Before refactoring I found some unused code.
I decided to not create standalone PRs for them.
They reside in standalone commits in this PR.

Removed extraneous

resolve: {
        extensions: ['', '.js', '.json’]

Removed unused
strategies/development.js with devtool: 'sourcemap’
instead of the right one source-map

Then.
Every refactoring requires tests first.

It is not a trivial task to test required or imported .js-files,
becuase of that I've created a simplest standalone test suit for this.

Then I filled test-cases up from all currently used webpack configurations.

Tests are green. Now we can refactor scripts itself.

The new scheme is simplest as possible.
There is base.conf.js and all other configs are based on it.
Command line args are parsed in base and then they are exported to children.

Note:
// Remove extract text plugin from dev workflow so hot reload works on css
here
Actually plugin was not been removing.
Tests made it plain that it was just a stale comment.

Tests are the same but configs are new. All is green.
Finally: I've added check for webpack configs into npm run test for TravisCI.

New webpack config generating scripts are much simpler and clean 🍒

And now about the IE8 configs.
IE8 dev part is totally broken.

Neither npm run ie8 command works, because of
Error: Cannot find module 'webpack-dev-middleware',
nor files like ie8/package.json are used.

I removed npm run ie8 command because it doesn't work now
and there is no fast fix for it at this stage.

I will create additional issue about IE8 state of things
where I will address all of it and some additional info.

AlexKVal · 2015-05-18T11:20:58Z

ref: issue about IE8 #692

mtscout6 · 2015-05-18T14:23:11Z

webpack/docs.config.js

+    noParse: /babel-core\/browser/,
+    loaders: [
+      { test: /\.js/, loader: `${reactHot}babel`, exclude: /node_modules|Samples\.js/ },
+      { test: /Samples.js/, loader: `${reactHot}transform?brfs!babel` },


One of the reasons I was doing a search for the loader I wanted to change was to allow changes in the base loader to cascade down. Since the base loader only has the one loader, how would you feel about including that as an export of the base and build these two on top of that one?

will do it via jsLoader

export const jsLoader = 'babel';

{ test: /\.js/, loader: `${reactHot}${jsLoader}`, exclude: /node_modules|Samples\.js/ }, { test: /Samples.js/, loader: `${reactHot}transform?brfs!${jsLoader}` },

Nice hint. Thank you!

mtscout6 · 2015-05-18T14:49:28Z

I think the testing strategy, while useful during your refactor, is a bit too much. The slightest change to the base config would cause all the test cases to fail meaning simple changes would require modifications to each test case. I'd advocate for a few targeted test cases that check sections of the configuration and not the entire configuration in one glob.

It would be nice if these tests were Mocha style tests, this way they can still be included in the existing test run.

mtscout6 · 2015-05-18T14:54:09Z

In general I welcome this change it is less complicated and easier to reason about. The initial approach I took was based on an internal implementation that we have on a much larger project. Which did not scale well here, so thanks for the good work on this @AlexKVal!

AlexKVal · 2015-05-19T17:15:41Z

Re-written.

I'd advocate for a few targeted test cases that check sections of the configuration and not the entire configuration in one glob.

I thought a lot about it before I created this PR.
The more I thought, the more I become convinced that we need to test every the configs as a whole.

The whole idea behind this refactoring is from my 1.5 month wathing on this project.
Only this month there were a handful changes in configs (and tools using them) that were led
to confusion and subtle errors (not discovered) every time. Let alone they stockpile.

I believe that there is no any small change that would lead to a lot of changes in 3 short text files 😈 . It's not burdensome, you can verify it 😉

But at the same time every change will reveal every possible side effect 👍
This test suit intended to help to make predictable changes in configs.

It would be nice if these tests were Mocha style tests.

I were tried it with karma as is, but existing tests run in browser.
To make them Mocha style possibly would cause in additional dependencies what I'm trying to avoid as much as possible.

I decided to post it as is to get some feedback first.
Now I can try to make some more research for Mocha style.
I'm not sure how many time it will get though.

What do you think if we merge this version - with proposed fixes - and then refactor it with Mocha.
Of course if there are no any objections.

I have a dread of additional changes in tools and configs in the meantime that would cause
in additional refactorings 😄
(as it already has taken place a couple times. sigh)

AlexKVal · 2015-05-19T17:23:46Z

Anyway I'm already learning how to refactor it with Mocha.

AlexKVal · 2015-05-19T19:20:45Z

Hah.. As it turned out it wasn't painful 😄
Mocha 🎉

actually only 98eb4c2 has been changed
and there only webpack/tests/index.js has been added instead of run-tests.
spec-files are the same
(exept I renamed
path => configFile
cases => tests
argv => args)

Added npm run webpack-check
"webpack-check": "mocha --require babelhook webpack/tests",

And yay.. we would have config diffs now

mtscout6 · 2015-05-20T02:17:59Z

babelhook.js

@@ -0,0 +1 @@
+require('babel/register');


Have you seen this: https://babeljs.io/docs/using-babel/#mocha

dozoisch · 2015-05-20T02:25:17Z

wow that new page is awesome (in babel)

mtscout6 · 2015-05-20T02:27:27Z

I still don't like the complete explicit configuration tests. If we are going to maintain them in such a verbose manner we may as well have the configs be that verbose and forego any kind of dynamic configuration. Those tests are incredibly brittle and will be required to change with every change to the base configuration. I'm ok with some unit tests that verify portions of the configurations like I've stated before, but I am not ok with them in the state they are now.

The point of testing is to provide a good safety net so you don't hurt yourself. Note that I said "safety net", and not "safety sail". Every a net has a few holes, but are still very effective at catching fish. While at the same time not preventing the boat from moving with the net on its tow line. Forgive the commercial fishing example, but I hope that conveys what I'm getting at.

AlexKVal · 2015-05-20T11:49:29Z

https://babeljs.io/docs/using-babel/#mocha

Didn't know it. Thank you 🍒

Edited: part about tests has been moved to #706 PR.

AlexKVal · 2015-05-20T12:02:44Z

I propose to split this PR into two.

I cut out into new PR #706 discussion about tests for webpack.

And here I left (squashed) just new config files 🍒

Remove some stale `IE8` pieces.

mtscout6 · 2015-05-20T15:56:00Z

LGTM

Get under control webpack config files.

AlexKVal · 2015-05-20T19:01:51Z

Yay 🎉 😃

…eact-bootstrap#691) Bumps [enzyme-adapter-react-16](https://github.com/enzymejs/enzyme/tree/HEAD/packages/enzyme-adapter-react-16) from 1.9.1 to 1.15.6. - [Release notes](https://github.com/enzymejs/enzyme/releases) - [Changelog](https://github.com/enzymejs/enzyme/blob/master/CHANGELOG.md) - [Commits](https://github.com/enzymejs/enzyme/commits/enzyme-adapter-react-16@1.15.6/packages/enzyme-adapter-react-16) Signed-off-by: dependabot-preview[bot] <support@dependabot.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

mtscout6 reviewed May 18, 2015
View reviewed changes

AlexKVal force-pushed the wpCnf branch from 50059e1 to 0633f6c Compare May 19, 2015 17:14

AlexKVal force-pushed the wpCnf branch from 0633f6c to 36e1bcd Compare May 19, 2015 19:19

mtscout6 reviewed May 20, 2015
View reviewed changes

babelhook.js

@@ -0,0 +1 @@

require('babel/register');

Copy link

Member

mtscout6 May 20, 2015

Choose a reason for hiding this comment

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

Have you seen this: https://babeljs.io/docs/using-babel/#mocha

AlexKVal force-pushed the wpCnf branch from 36e1bcd to 625fbe4 Compare May 20, 2015 12:00

AlexKVal force-pushed the wpCnf branch from 625fbe4 to 63c74c7 Compare May 20, 2015 12:03

Simplify webpack config files.

2a1d7f6

Remove some stale `IE8` pieces.

AlexKVal force-pushed the wpCnf branch from 63c74c7 to 2a1d7f6 Compare May 20, 2015 12:07

This was referenced May 20, 2015

[Do not merge. Draft] Partial testing of webpack config files. #706

Closed

Make webpack.config for npm run build based on the bower dependencies. #707

Closed

mtscout6 added a commit that referenced this pull request May 20, 2015

Merge pull request #691 from AlexKVal/wpCnf

1b58489

Get under control webpack config files.

mtscout6 merged commit 1b58489 into react-bootstrap:master May 20, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get under control webpack config files. #691

Get under control webpack config files. #691

AlexKVal commented May 18, 2015

AlexKVal commented May 18, 2015

mtscout6 May 18, 2015

AlexKVal May 19, 2015

mtscout6 May 19, 2015

mtscout6 commented May 18, 2015

mtscout6 commented May 18, 2015

AlexKVal commented May 19, 2015

AlexKVal commented May 19, 2015

AlexKVal commented May 19, 2015

mtscout6 May 20, 2015

dozoisch commented May 20, 2015

mtscout6 commented May 20, 2015

AlexKVal commented May 20, 2015

AlexKVal commented May 20, 2015

mtscout6 commented May 20, 2015

AlexKVal commented May 20, 2015

Get under control webpack config files. #691

Get under control webpack config files. #691

Conversation

AlexKVal commented May 18, 2015

AlexKVal commented May 18, 2015

mtscout6 May 18, 2015

Choose a reason for hiding this comment

AlexKVal May 19, 2015

Choose a reason for hiding this comment

mtscout6 May 19, 2015

Choose a reason for hiding this comment

mtscout6 commented May 18, 2015

mtscout6 commented May 18, 2015

AlexKVal commented May 19, 2015

AlexKVal commented May 19, 2015

AlexKVal commented May 19, 2015

mtscout6 May 20, 2015

Choose a reason for hiding this comment

dozoisch commented May 20, 2015

mtscout6 commented May 20, 2015

AlexKVal commented May 20, 2015

AlexKVal commented May 20, 2015

mtscout6 commented May 20, 2015

AlexKVal commented May 20, 2015