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

Upgrade to webpack 3 and add webpack-dashboard. #5616

Closed
wants to merge 20 commits into from
Closed

Conversation

rxl881
Copy link
Contributor

@rxl881 rxl881 commented Nov 16, 2017

... I was getting annoyed with the ancient version of webpack 1 that we are currently using, especially the interminable "optimising asset chunks", and wondered if we could either speed things up or get a better insight in to what webpack is doing by upgrading to a more recent version and some webpack bundle analysis tools.

In this PR I have upgraded webpack from "^1.12.14" to latest stable ("^3.8.1").

I have also added support for webpack-dashboard (https://github.com/FormidableLabs/webpack-dashboard), which appears to give some reasonable insight in to the webpack build process, bundle size and any potential problems.

By default the PR does not introduce any changes to the build / run process. However, if desired, you can run in dashboard mode with "npm/yarn run dashboard".

As this is a fairly major increase in webpack versions / dependencies, it probably needs some fairly rigorous testing by other devs. before choosing if we want to merge it to /develop. @dbkr, @ara4n, can you have a quick play when you have a sec.? Thanks.

Some screenshots of webpack-dashboard running riot-web:

screen shot 2017-11-16 at 10 18 58

screen shot 2017-11-16 at 10 19 29

screen shot 2017-11-16 at 10 21 15

@rxl881 rxl881 assigned dbkr and ara4n Nov 16, 2017
@rxl881
Copy link
Contributor Author

rxl881 commented Nov 16, 2017

Hmm... it seems that the travis CI build is failing due to being unable to find matrix-react-sdk and its deps. (the tests works fine in dev. env.):

ERROR in ./matrix-js-sdk/lib/crypto/olmlib.js
Module not found: Error: Can't resolve 'another-json' in '/home/travis/build/vector-im/riot-web/matrix-js-sdk/lib/crypto'
 @ ./matrix-js-sdk/lib/crypto/olmlib.js 96:18-41
 @ ./matrix-js-sdk/lib/crypto/index.js
 @ ./matrix-js-sdk/lib/client.js
 @ ./matrix-js-sdk/lib/matrix.js
 @ ./matrix-js-sdk/browser-index.js
 @ ./test/app-tests/joining.js
 @ ./test/app-tests \.jsx?$
 @ ./test/all-tests.js

@dbkr any idea what bits of karma.conf.js need to be twiddled to fix this?

@lukebarnard1
Copy link
Contributor

FTR, I've seen travis missing arbitrary dependencies before. Solution was to re-run the tests. Could be wrong.

Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

I'd guess the reason this isn't working is because you're adding node_moudles as an absolute path which (for some reason) make it non-recursive (https://webpack.js.org/configuration/resolve/#resolve-modules). Are the tests working on yours?

package.json Outdated
@@ -42,10 +42,11 @@
"install:electron": "install-app-deps",
"electron": "npm run install:electron && electron .",
"start:res": "node scripts/copy-res.js -w",
"start:js": "webpack-dev-server --output-filename=bundles/_dev_/[name].js --output-chunk-file=bundles/_dev_/[name].js -w --progress",
"start:js": "webpack-dev-server --output-filename=bundles/_dev_/[name].js --hot --inline -w --progress",
Copy link
Member

Choose a reason for hiding this comment

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

are these not duplications of the dev server options below?

@dbkr dbkr assigned rxl881 and unassigned dbkr Nov 20, 2017
@MTRNord
Copy link
Contributor

MTRNord commented Nov 20, 2017

There is another webpack 3 pr I made some time ago in this repo. You might look what I did there to get the node_modules working

@MTRNord
Copy link
Contributor

MTRNord commented Nov 20, 2017

(my pr is #4800)

@rxl881
Copy link
Contributor Author

rxl881 commented Nov 20, 2017

Ahh... thanks for pointing that out @MTRNord, I hadn't seen that one! I'll check with @dbkr how best to proceed with these ones. 👍

@MTRNord
Copy link
Contributor

MTRNord commented Nov 20, 2017

I would have no problem if this gets merged instead of mine :) (as this got more New stuff than mine)

@rxl881 rxl881 changed the title Upgrade to webapck 3 and add webpack-dashboard. Upgrade to webpack 3 and add webpack-dashboard. Nov 20, 2017
@rxl881
Copy link
Contributor Author

rxl881 commented Nov 21, 2017

I'm not going to spend any more time on this for the time being... It's all working fine on my local dev. env, but unfortunately I'm unable to get the Travis CI tests to pass for node 6 / 7 (appears to be consistently working for node 8).

@MTRNord - for whatever reason your PR seems to be passing the tests fine, but I can't see the difference. Please let me know if you have any ideas (on what the problem might be and how to fix it). As you mentioned earlier, this PR has a few new features including webpack-dashboard, so it would have been good to use it, but I'm happy to dump this and go with your original PR, if we can't fix it easily. @dbkr is probably best placed to have a look at your PR, so I've assigned him as a reviewer there too. Sorry that your PR was dropped for so long, hopefully we can get one of these pushed through soon.

@MTRNord
Copy link
Contributor

MTRNord commented Nov 21, 2017

@rxl881 well I don't have a idea either tbh. I looked at the tests and cant figure out what is failing (as some stuff even works). Wither way It would be a good idea to re run the tests on my PR as it is pretty old already.

@rxl881
Copy link
Contributor Author

rxl881 commented Nov 21, 2017

Thanks @MTRNord - I tried re-running the tests on against your PR a couple of times to see if I could get them to fail ... they didn't! Weird, as I really can't see the obvious difference.

@MTRNord
Copy link
Contributor

MTRNord commented Nov 21, 2017

@rxl881 the only difference I see is that I ensure that olm.js gets loaded at the correct position in the html file and that you did update karma but I did not

@rxl881
Copy link
Contributor Author

rxl881 commented Nov 21, 2017

@MTRNord - Yeah, I couldn't see why you were changing the loading position of olm.js and if it was related (would be good to know the background on that one, if you can remember it).

Interesting point about not updating karma. I'll try reverting that and re-running the tests now. If that doesn't work, I'll try adding in the olm change... and if that doesn't work, I really will leave it ;)

@MTRNord
Copy link
Contributor

MTRNord commented Nov 21, 2017

@rxl881 the olm.js script sometimes did get loaded too late when building locally. On CI that never was a problem only when building local. The array returned from seems to sometimes not have the right order. I don't know exactly what it was I will see if I get the chat log from the riot-dev room

@MTRNord
Copy link
Contributor

MTRNord commented Nov 21, 2017

@rxl881 original discussion on olm.js loading is here: https://riot.im/develop/#/room/#riot-dev:matrix.org/$15028345341431mZAch:sw1v.org

@MTRNord
Copy link
Contributor

MTRNord commented Nov 21, 2017

Hm comparing the tests mine builds webpack 2 times for some reason this one doesn't. I wonder if that makes a difference and why karma does that. But I am out of ideas how to fix the problem

@rxl881
Copy link
Contributor Author

rxl881 commented Nov 21, 2017

Thanks @MTRNord - The olm.js change makes a lot of sense now (I've added in your patch here).

It's looking more and more (to me) like the issues that I was seeing are probably due to the karma upgrade (as you suggested). I've tried rolling back most of the deps. However, the old version of karma now doesn't seem to be playing nicely with the modified webpack3 config... can of worms.

... ugghh... CI build has just failed again ...

I suggest that we go with your original PR (hopefully @dbkr can look at it soon) and then I / we can have another look at upgrading incrementally from there, adding back in webpack-dashboard etc.

Closing this PR for now.

@rxl881 rxl881 closed this Nov 21, 2017
@rxl881 rxl881 deleted the rxl881/dashboard branch April 3, 2018 21:23
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.

5 participants