Webpack all the things #640

nornagon · 2016-09-25T17:11:02Z

Builds on top of #609. Drops uglify in favor of webpack. Uses code splitting and imports-loader to get everything to play nicely together.

astorije · 2016-09-27T05:37:29Z

So, a word of caution: I believe some of the libs in client/js/libs have been modified in the repo (actually, I know some have diverged, but I don't see them in this diff). Did you check the potential differences when removing the files?

xPaw · 2016-09-27T07:40:03Z

@astorije the removed libs were not modified by us

nornagon · 2016-09-27T16:52:46Z

$ git log --stat client/js/libs/handlebars.js client/js/libs/jquery.js client/js/libs/jquery/jquery-ui.js client/js/libs/jquery/mousetrap.js client/js/libs/notification.js client/js/libs/uri.js

^- only contains large deltas, which suggests that those libraries were only ever added or updated.

astorije

A few minor changes and some naive and stupid questions as I have never used webpack, but good stuff overall!

astorije · 2016-09-30T04:19:56Z

webpack.config.js

+module.exports = {
+	entry: "./client/js/lounge.js",
+	resolve: {
+		extensions: ["", ".js"]


Same question than #640 for the record: why do we need "" here?

So it allows importing without the ".js" extension in import.

Aaah, makes sense! Do we want to allow both with and without the extension? Or force one or the other format?

As per our discussion on IRC, could you remove that section altogether and rely on the defaults?

Yep, looks like that works fine!

astorije · 2016-09-30T04:22:32Z

.gitignore

@@ -7,6 +7,5 @@ coverage/
 # See https://docs.npmjs.com/misc/scripts
 client/fonts/
 client/js/bundle.js
-client/js/libs.min.js.map


No .map anymore? Is that on purpose?

This webpack config doesn't produce source maps, but no reason it couldn't--I'll update it to produce inline source maps (so no need for an extra file).

Do not inline the source map. Thats a lot of useless data to be included.

astorije · 2016-09-30T04:27:02Z

client/js/libs/handlebars/index.js

+import "./roundBadgeNumber";
+import "./tojson";
+import "./tz";
+import "./users";


I sure wish there was a way to do import "./*"; but apparently there is no such thing.

Nope, quite intentionally :) (That would cause an import cycle, for one thing!)

Well, you could locate the loading file outside of the directory (Ruby-style). But yeah, whatever :-)

astorije · 2016-09-30T04:31:56Z

client/js/lounge.js

+import "jquery-ui/ui/widgets/sortable";
+import Mousetrap from "mousetrap";
+import Handlebars from "handlebars/runtime";
+import "notification-polyfill";


Is that polyfill still necessary?

http://caniuse.com/#feat=notifications hm, not sure.

Ok maybe let's keep it for now then. Sigh...

Seems like IE still doesn't have notification support, so yeah.

astorije · 2016-09-30T04:34:45Z

client/js/lounge.js

+
+import "./libs/jquery/inputhistory";
+import "./libs/jquery/tabcomplete";
+import "./libs/jquery/stickyscroll";


I believe all these libs by @erming are on npm, but also that we have monkey-patched these over time. A great contribution would be diff-ing ours with the upstream ones (because, yeah, they have evolved on the upstream project too...), open PRs on @erming's repos, and as they are being merged, move these to use the npm versions :-)

I intend to ultimately remove all of these libs as dependencies.

@astorije These "libraries" were so heavily modified that I would consider them part of Lounge's code and not vendor.

astorije · 2016-09-30T04:38:08Z

package.json

    "mocha": "3.0.2",
+    "mousetrap": "1.6.0",


Same comment, version in repo is 1.4.6.

Good catch.

astorije · 2016-09-30T04:38:27Z

package.json

    "npm-run-all": "3.1.0",
    "stylelint": "7.3.1",
-    "uglify-js": "2.7.3",
+    "urijs": "1.18.1",


Version in repo is 1.14.1.

NPM only has back to 1.16.1 of this lib, so I've rolled back to that.

astorije · 2016-09-30T04:40:18Z

src/client.js

@@ -1,5 +1,5 @@
 var _ = require("lodash");
-var package = require("../package.json");
+var packageJson = require("../package.json");


#399 did something similar with server code, but went with pkg. I have no preference over which one, but it would be nice to use the same one :-)

astorije · 2016-09-30T04:42:22Z

package.json

@@ -64,11 +63,16 @@
    "eslint": "3.6.0",
    "font-awesome": "4.6.3",
    "handlebars": "4.0.5",
+    "imports-loader": "0.6.5",


Not sure I'm getting this, why do we need it?

To make $ appear in libs without calling require.

Good point, didn't think of that. Clever way to do so too, cool!

I think we should explicitly require the needed stuff without automagic.

I don't think this is even possible or reasonable. Unless I am mistaken, I believe @nornagon is talking about real libs (like jquery-ui) and not our "libs" in client/js/libs.

Or maybe there is a different/better way to do so, I don't know. I think this is good enough for a first shot at it and we can improve later.

@nornagon I see you added imports for $ and others, is this still needed?

imports-loader is still being used to provide the jQuery global variable to the jquery libs, which I haven't changed in this diff.

astorije · 2016-09-30T04:42:58Z

webpack.config.js

@@ -1,5 +1,17 @@
+const webpack = require("webpack");


Interesting, why do you need to require webpack in your second commit? It seems like it was not needed for the first one.

Because this one uses webpack in its body, to refer to webpack.optimize.CommonsChunkPlugin.

Duh yeah, I missed that (aka reviewing PRs at 1AM 😅).
Too bad we can't use import here though, the irony :-)

astorije · 2016-09-30T05:41:02Z

Let me know when you're done updating this :-)

nornagon · 2016-09-30T22:52:40Z

Ready for another look!

astorije

1 minor comment and 1 very small change and then we should be ok :) (I'll test after that before approving this though).

astorije · 2016-10-01T05:37:15Z

package.json

    "istanbul": "0.4.5",
+    "jquery": "2.1.1",
+    "jquery-ui": "1.12.1",


Did 1.12.1 work as expected? Did the version bump turn out to fix #494 by any miracle?

astorije · 2016-10-01T05:50:02Z

webpack.config.js

+module.exports = {
+	entry: "./client/js/lounge.js",
+	resolve: {
+		extensions: ["", ".js"]


As per our discussion on IRC, could you remove that section altogether and rely on the defaults?

xPaw · 2016-10-01T06:27:47Z

package.json

    "build:handlebars": "handlebars client/views/ -e tpl -f client/js/lounge.templates.js",
+    "build:webpack": "webpack",
+    "watch": "webpack -w",


Can we also get a script to automatically start the server and webpack's watch within a single command for development purposes?

xPaw · 2016-10-01T06:29:31Z

src/client.js

@@ -1,5 +1,5 @@
 var _ = require("lodash");
-var package = require("../package.json");
+var pkg = require("../package.json");


You touched client.js for no reason.

This isn't for no reason. package is a reserved word in ES6, and webpack was refusing to compile this file without this rename.

Also, if you add "use strict"; at the top (and feel free to do so on files you changed as it's a really good thing to have), implements, interface, let, package, private, protected, public, static, and yield become reserved keywords.

xPaw · 2016-10-01T06:30:20Z

webpack.config.js

+const webpack = require("webpack");
+
+module.exports = {
+	entry: {


Just for future proofing, will it be easy to add our postcss stuff into this config, or it will have to be rewritten for an additional export or whatever?

Yep, should be easy to add. See https://github.com/postcss/postcss-loader.

astorije

Awesome job, @nornagon! Alright people, let's ! And improve along the way when necessary.

xPaw · 2016-10-02T09:31:08Z

package.json

@@ -14,16 +14,16 @@
  "scripts": {
    "coverage": "istanbul cover node_modules/mocha/bin/_mocha -r test/fixtures/env.js test/**/*.js",
    "start": "node index",
+    "start-dev": "npm run watch & npm run start",


This only runs the webpack watch and blocks, and then never runs the lounge itself.

That's not how & works (perhaps you're thinking of &&?)

xPaw

There's a couple of things that need to be addressed:

babel-core has to be added to package.json:

ERROR in Cannot find module 'babel-core'
npm WARN [email protected] requires a peer of babel-core@^6.0.0 but none was installed.

Vendor file contains inline source maps,and the file is absolutely huge (554kb!!), generate an external .map.

Source map not being generated for bundle.js (make it external too)

inputhistory requires jQuery, but the dependancy is not resolved: Uncaught ReferenceError: jQuery is not defined

start-dev only runs webpack and doesn't start the server.

I think uglifyjs should be kept to generate a lot smaller files (especially vendor). bundle.js drops at least half in size with uglify plugin.

xPaw · 2016-10-02T10:56:03Z

Also, this needs to be rebased on master due to merge of slideout menu (new libs/slideout.js file)

astorije · 2016-10-06T05:57:18Z

@nornagon, any plans to address @xPaw's comments soon? I would love to see this PR merged :))

nornagon · 2016-10-09T22:06:48Z

Added babel-core. Whoops.

Source maps inlined in vendor.bundle.js are there in the upstream code, it's not webpack that's adding them. The config in this diff wasn't generating source maps at all for our own code; I've tweaked it to generate them and put them in separate files. With this change:

$ du -sh client/js/*bundle*
112K    client/js/bundle.js
300K    client/js/bundle.js.map
560K    client/js/vendor.bundle.js
1.4M    client/js/vendor.bundle.js.map

I experimented with uglify and it did significantly drop the file sizes:

$ du -sh client/js/*bundle.js
 56K    client/js/bundle.js
180K    client/js/vendor.bundle.js

but at the cost of 2x build time:

$ time npm run build:uglify-webpack
[...]
npm run build:uglify-webpack  8.30s user 0.32s system 110% cpu 7.789 total
$ time npm run build:webpack
[...]
npm run build:webpack  4.05s user 0.26s system 114% cpu 3.771 total

so I don't think it makes sense to uglify during dev. Many projects using webpack have a separate config for prod vs. dev, and I'd be happy to work on that, but perhaps it could be in a followup PR?

I can't reproduce your issue with inputhistory. I tried building from a clean repo and I don't see any error. This is the code webpack is generating:

    /*** IMPORTS FROM imports-loader ***/
    var jQuery = __webpack_require__(1);

    /*!
     * inputhistory
     * https://github.com/erming/inputhistory
     * v0.3.1
     */
    (function ($) {
        $.inputhistory = {};
        /* ... */
    })(jQuery);

which seems fine to me. In addition, the functionality (pressing up to get to old messages) works just fine.

I also can't reproduce your issue with start-dev, it works fine for me:

$ npm run start-dev

> [email protected] start-dev /Users/nornagon/Source/lounge
> npm run watch & npm run start


> [email protected] watch /Users/nornagon/Source/lounge
> webpack -w


> [email protected] start /Users/nornagon/Source/lounge
> node index

2016-10-09 22:05:04 [INFO] The Lounge v2.0.0 is now running on http://*:9000/ in public mode
2016-10-09 22:05:04 [INFO] Press ctrl-c to stop

Hash: 2e8283f667df32ce2724
Version: webpack 1.13.2
Time: 3009ms
               Asset    Size  Chunks             Chunk Names
           bundle.js  112 kB       0  [emitted]  app
    vendor.bundle.js  573 kB       1  [emitted]  vendor
       bundle.js.map  228 kB       0  [emitted]  app
vendor.bundle.js.map  688 kB       1  [emitted]  vendor
   [0] multi vendor 88 bytes {1} [built]
    + 50 hidden modules

nornagon · 2016-10-09T22:14:06Z

Also rebased, and used import instead of side-effecting onto window to import the slideoutMenu function.

nornagon · 2016-10-09T23:04:53Z

Just realised the start-dev issue might be a windows thing, are you testing on windows @xPaw ?

astorije · 2016-10-22T19:57:48Z

client/js/libs/jquery/tabcomplete.js

 	var keys = {
 		backspace: 8,
 		tab: 9,
 		up: 38,
 		down: 40
 	};
-	
+


Let's not lint these, I still want them to come from upstream project (so, npm) after backporting our changes. I started on one, need to continue.

williamboman · 2016-10-25T15:57:13Z

client/js/libs/handlebars/tojson.js

-);
+module.exports = function(context) {
+	return window.JSON.stringify(context);
+};


This file needs to be renamed to toJSON.js, otherwise the handlebars loader will be unable to resolve it on case-sensitive file systems.

xPaw · 2016-11-20T08:35:19Z

@astorije I've reverted changes to erming's libs so they are untouched by this PR. Also squashed it.

Fixes thelounge#426.

astorije · 2016-11-20T19:32:54Z

Also squashed it.

Nooooo, I was still trying to test some things, which are now gone as you've reverted them as part of one of the commits. Can you revert this, please? (git reflog for the win)
EDIT: I checked and I have no kind of access whatsoever to the unsquashed version, so all of what I had been doing that you reverted has been lost. Please revert the squash.

AlMcKinlay · 2016-11-24T16:30:36Z

This will need rebased again. What's the plan with this, it would be great to have this, then we can start splitting up our monolithic codebase.

xPaw · 2016-11-24T16:48:26Z

@YaManicKill astorije wanted something in this PR before merge.

astorije · 2016-11-27T21:49:35Z

@xPaw, reminder to un-squash this for now please? :)

xPaw · 2016-12-09T16:24:11Z

@astorije sorry I am not able to un-squash this.

AlMcKinlay · 2016-12-09T16:28:32Z

One of you should have it on your reflog, and if not, the server should hav eit on the reflog, no?

astorije · 2016-12-10T07:01:44Z

Uuuuh so you mean what I spent 1-2 days working on is lost? Please no!
I don't have the repo under which I worked on anymore, I was not expecting it to be squashed.
@xPaw, as @YaManicKill said, can you browse your reflog?

xPaw · 2016-12-10T07:45:54Z

@astorije fortunately not lost! This should be it: https://github.com/nornagon/lounge/commits/291aa975c5a9847404b7fa928b3959773f2a5a7a

For reference: https://8thlight.com/blog/sandro-padin/2015/06/08/help-i-just-force-pushed-to-master.html

AlMcKinlay · 2016-12-12T12:18:24Z

@astorije Are the things you were testing related to this PR, or are we happy to go with this now? Would be good to get this merged asap, so that other people building PRs can build off this rather than the old code.

AlMcKinlay · 2016-12-12T12:19:13Z

@nornagon You've still got some lint errors. Could you fix these and rebase? Would be great to get this into a release soon.

PolarizedIons · 2016-12-13T20:18:11Z

Trying to run this PR (via ./scripts/run_pr.sh 640, though I did do npm install and npm run build manually just to be sure) and get this in the console when trying to load thelounge:

The page is also stuck on "Loading the app…"

xPaw · 2016-12-18T15:56:15Z

I've pushed a fixed branch under our repo: https://github.com/thelounge/lounge/tree/pr/640

It's a lot easier to deal with the branch within the repo rather than dealing with the fork.

xPaw · 2016-12-18T16:08:13Z

Closed in favour of fixed #817.

nornagon force-pushed the webpack-all-the-things branch from 5efab30 to a7718fa Compare September 25, 2016 21:50

astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Sep 27, 2016

nornagon force-pushed the webpack-all-the-things branch from a7718fa to 2c4a924 Compare September 27, 2016 18:40

astorije mentioned this pull request Sep 30, 2016

Add babel and webpack to the build process to allow ES2015 #609

Closed

astorije requested changes Sep 30, 2016

View reviewed changes

astorije self-assigned this Sep 30, 2016

nornagon force-pushed the webpack-all-the-things branch from 2c4a924 to e67e0e3 Compare September 30, 2016 22:29

astorije requested changes Oct 1, 2016

View reviewed changes

xPaw reviewed Oct 1, 2016

View reviewed changes

nornagon force-pushed the webpack-all-the-things branch from e67e0e3 to f06476c Compare October 2, 2016 00:52

astorije approved these changes Oct 2, 2016

View reviewed changes

astorije added the second review needed label Oct 2, 2016

xPaw added this to the 2.1.0 milestone Oct 2, 2016

xPaw reviewed Oct 2, 2016

View reviewed changes

xPaw requested changes Oct 2, 2016

View reviewed changes

astorije mentioned this pull request Oct 2, 2016

Add postcss #630

Closed

nornagon force-pushed the webpack-all-the-things branch from f06476c to 51f9cdd Compare October 9, 2016 22:05

nornagon force-pushed the webpack-all-the-things branch from 51f9cdd to 352a1b9 Compare October 9, 2016 22:13

xPaw removed the Meta: Do Not Merge This PR should not be merged. label Oct 22, 2016

astorije requested changes Oct 22, 2016

View reviewed changes

astorije force-pushed the webpack-all-the-things branch 2 times, most recently from f31a45b to 3b0d324 Compare October 23, 2016 05:31

williamboman reviewed Oct 25, 2016

View reviewed changes

xPaw force-pushed the webpack-all-the-things branch from 291aa97 to 37c29f3 Compare November 20, 2016 08:32

xPaw approved these changes Nov 20, 2016

View reviewed changes

Add babel and webpack to the build process to allow ES6

4198207

Fixes thelounge#426.

xPaw force-pushed the webpack-all-the-things branch from 37c29f3 to 4198207 Compare November 20, 2016 08:36

Merge branch 'master' into webpack-all-the-things

6827873

Bonuspunkt mentioned this pull request Dec 10, 2016

improve client message parsing #699

Closed

Merge branch 'master' into webpack-all-the-things

6354289

xPaw removed the priority label Dec 16, 2016

xPaw mentioned this pull request Dec 18, 2016

Webpack #817

Merged

xPaw closed this Dec 18, 2016

MaxLeiter mentioned this pull request Dec 18, 2016

Add autocomplete for emoji, users, chans, and commands #787

Merged

Webpack all the things #640

Webpack all the things #640

Conversation

nornagon commented Sep 25, 2016

astorije commented Sep 27, 2016

xPaw commented Sep 27, 2016

nornagon commented Sep 27, 2016

astorije left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented Sep 30, 2016

nornagon commented Sep 30, 2016

astorije left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nornagon Oct 9, 2016 • edited Loading

Choose a reason for hiding this comment

xPaw left a comment

Choose a reason for hiding this comment

xPaw commented Oct 2, 2016

astorije commented Oct 6, 2016

nornagon commented Oct 9, 2016

nornagon commented Oct 9, 2016

nornagon commented Oct 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xPaw commented Nov 20, 2016

astorije commented Nov 20, 2016 • edited Loading

AlMcKinlay commented Nov 24, 2016

xPaw commented Nov 24, 2016

astorije commented Nov 27, 2016

xPaw commented Dec 9, 2016

AlMcKinlay commented Dec 9, 2016

astorije commented Dec 10, 2016

xPaw commented Dec 10, 2016

astorije Sep 30, 2016 •

edited

Loading

nornagon Oct 9, 2016 •

edited

Loading

astorije commented Nov 20, 2016 •

edited

Loading

PolarizedIons commented Dec 13, 2016 •

edited

Loading