Webpack #817

xPaw · 2016-12-18T15:57:14Z

All discussion in #640. This PR is finalized and ready.

Separate PR because I moved the branch to our repo for easier fixing (don't have to deal with the fork repo).

AlMcKinlay

Yeah, seems to work great, let's get this in. I want to modularise things :-P

astorije

Argh, so sorry, I wrote this a couple days ago and it stayed in "Pending" mode, dammit.

astorije · 2016-12-20T07:25:16Z

package.json

    "chai": "3.5.0",
    "eslint": "3.10.2",
    "font-awesome": "4.7.0",
    "handlebars": "4.0.6",
+    "handlebars-loader": "1.4.0",


Not trying to compare with our current way of running npm run build:handlebars for our helpers/templates, but I am having a hard time loving this dependency, am I the only one?

Essentially, what bothers me is that because handlebars-loader comes with its opinionated (as in, it differs from the vanilla Handlebars) format, we now totally depend on it to run the code, and therefore creates a strong tie between the code and Webpack (a module bundler), which is less than ideal. Ideally, what concerns the packaging of the app doesn't have an effect on the code itself.

In a perfect world, I would like our Handlebars helpers to follow the Handlebars way (so using Handlebars.registerHelper), same for templates, and Webpack finds its way around it, whether it's through an import Handlebars, or whatever.
Does that make sense? Cut the middleman and use Handlebars syntax/features/docus, but still benefit from Webpacking stuff.
Now, I'm not saying such solution exists out of the box, but on the other PR my tests were starting to show success just before handlebars-loader was introduced.

comes with its opinionated (as in, it differs from the vanilla Handlebars) format

Does it?

I think we benefit a lot more from using a well test covered loader that solves some of the pain points we hit during creation of this PR. What this allows us to do is import templates directly, and the returned template is already compiled ready for consumption. Helpers are turned into generic functions which can also be used by other parts of our code without hitting Handlebars at alal.

I'm with @xPaw on this one. The new format is actually more generic (the old one binds directly to handlebars, this one uses standard module formats and just does the job).

Also makes them infinitely more testable.

Alright, I'll go with the flow. I agree that there are some clear advantages to it despite my concerns.

astorije

Good stuff overall, a few comments inline regarding npm install not passing on a clean repo.

npm run build takes 15s+, most of it being Webpack. Meh, could be worse, but I didn't think it would take more than a couple seconds.

astorije · 2016-12-24T22:58:05Z

.gitignore

+client/js/bundle.js
+client/js/bundle.js.map
+client/js/bundle.vendor.js
+client/js/bundle.vendor.js.map


Shouldn't we add client/js/*.map to the .npmignore file too? I'm not sure we want to distribute the maps with the package / client.

54K bundle.js 308K bundle.js.map 247K bundle.vendor.js 1.9M bundle.vendor.js.map

(We do serve maps at the moment, but they're only 300k total)

I think we can ignore vendor bundle map. Map for our own code is useful to have.

astorije · 2016-12-24T23:30:09Z

package.json

@@ -59,14 +58,22 @@
    "ldapjs": "1.0.1"
  },
  "devDependencies": {
+    "babel-core": "6.21.0",
+    "babel-loader": "6.2.10",
+    "babel-preset-es2015": "6.18.0",
    "chai": "3.5.0",
    "eslint": "3.12.2",
    "font-awesome": "4.7.0",
    "handlebars": "4.0.6",


So far handlebars in devDependencies was used just for testing, but now it is used to build the bundle. Version we had in the repo was v4.0.5. Are we ok going with v4.0.6 as part of this PR or play it safe and go one change at a time?

FYI diff is at handlebars-lang/handlebars.js@v4.0.5...v4.0.6.

Handlebars was bumped in 6023035 (master), not in this PR.

Nah, that was Handlebars used by the helpers tests, but the version in repo, i.e. the one currently used by the client is v4.0.5.

Right, client has been tested on 4.0.6 already and it doesn't break.

astorije · 2016-12-24T23:43:53Z

package.json

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


There is a massive diff between 1.11.1 (version in currently in repo) and 1.12.1. I tried and we can't use the tag directly from GitHub so I guess we'll have to go with that.
I don't think we use a lot from jQuery UI anyway, this can probably go at some point...

It's only used for sorting channels.

Aka a feature that one person out of 10 discovers months after using the app :D

astorije · 2016-12-24T23:55:12Z

client/js/lounge.js

+// vendor libraries
+import "jquery-ui/ui/widgets/sortable";
+import $ from "jquery";
+import io from "socket.io-client";


At the moment, this does build (at least on my machine), and I'm very confused why it builds on Travis CI (at least for the version of Node/npm we have in common).

I get this at the end of npm install on a clean repo (git clone https://github.com/thelounge/lounge && cd lounge && git checkout pr/640 && npm i):

[0] multi vendor 88 bytes {1} [built] [1 error] + 75 hidden modules ERROR in multi vendor Module not found: Error: Cannot resolve module 'socket.io-client' in /private/tmp/lounge @ multi vendor ERROR in ./client/js/lounge.js Module not found: Error: Cannot resolve module 'socket.io-client' in /private/tmp/lounge/client/js @ ./client/js/lounge.js 9:14-41

App does not run in the browser, I get this in the browser console:

bundle.vendor.js:105 Uncaught Error: Cannot find module "socket.io-client" at bundle.vendor.js:105 at Object.<anonymous> (bundle.vendor.js:105) at t (bootstrap 0a4ff42…:50) at bootstrap 0a4ff42…:93 at bootstrap 0a4ff42…:93 lounge.js:4 Uncaught Error: Cannot find module "socket.io-client" at lounge.js:4 at Object.<anonymous> (lounge.js:4) at t (bootstrap 0a4ff42…:50) at window.webpackJsonp (bootstrap 0a4ff42…:21) at bundle.js:1

I replaced this line with import io from "socket.io/lib/client"; to fix it.
An alternative is to add socket.io-client in package.json but I'd rather count on the version provided by the server. I'll add this in an extra commit to see if Travis is happy with it, let me know what you think.

That's the same issue I hit in the server tests PR.

astorije · 2016-12-25T00:28:56Z

Alright, I give up, I am unable to push to this branch. Either Git thinks I created a new branch or I get fatal: pr/640 cannot be resolved to branch.. Branch name at the top of this PR is pr/640 and in my working copy, it comes from upstream as origin/PR/640. First time I've ever had that crappy issue...

Anyway, diff I was trying to push:

───────────────────────────────────────────────────────────────────────────────────────
modified: client/js/lounge.js
───────────────────────────────────────────────────────────────────────────────────────
@ lounge.js:4 @
 // vendor libraries
 import "jquery-ui/ui/widgets/sortable";
 import $ from "jquery";
-import io from "socket.io-client";
+import io from "socket.io/lib/client";
 import Mousetrap from "mousetrap";
 import URI from "urijs";

───────────────────────────────────────────────────────────────────────────────────────
modified: webpack.config.js
───────────────────────────────────────────────────────────────────────────────────────
@ webpack.config.js:14 @ module.exports = {
            "jquery",
            "jquery-ui/ui/widgets/sortable",
            "mousetrap",
-           "socket.io-client",
+           "socket.io/lib/client",
            "urijs",
        ],
    },

astorije

Alright, I think that with the few fixes I've made, we should be all set (well, one minor nitpick, I'll let you decide).

@xPaw, before merging, can you squash my commits into your first one, so we end up with your 2 commits, clean, with this PR?

astorije · 2016-12-26T05:25:41Z

package.json

@@ -14,16 +14,15 @@
  "scripts": {
    "coverage": "istanbul cover node_modules/mocha/bin/_mocha",
    "start": "node index",
+    "start-dev": "npm-run-all --parallel watch start",


Minor: What about start:watch instead?

start-dev is easier to type, imo

xPaw · 2016-12-26T07:26:17Z

@astorije So I think just listing socket.io-client dependency is a lot more cleaner (we do depend on it), same issue was in #838 where I listed it.

xPaw · 2016-12-26T15:28:24Z

By the way, final JS size decreases:

master: lounge.js + lounge.templates.js + libs.min.js + socket.io.js = 389.8 kB
webpack: bundle.js + bundle.vendor.js = 313.8 kB

astorije · 2016-12-26T20:33:23Z

So I think just listing socket.io-client dependency is a lot more cleaner (we do depend on it)

It's not less clean in any way. And actually, socket.io even exposes /lib/ and its content. We don't depend on socket.io-client, we depend on socket.io and the client it exposes.
socket.io-client is useful when you are building a client-only app, where the server is another project/repo and you just want a client lib to connect to it. In our case, we are doing both, we have no interest in manually keeping track of a disconnected socket.io-client since the server already does it for us.

Also, this is not acceptable as long as we support npm v2: specifying both socket.io and socket.io-client will cause the latter to be installed twice, which we certainly don't want.

xPaw · 2016-12-27T20:24:45Z

For history: @astorije's method was to use "socket.io/lib/client", but that code is not the client code that can run in browser, it's some client instance code for the server. The serveClient method serves a JS directly from socket.io-client package.

astorije · 2016-12-28T06:42:38Z

@xPaw, kind of a good news, I just realized that serving the minified client is only done as of socket.io v1.7.0 (socketio/socket.io#2766) released in November 27.
Greenkeeper has updated the lib from 1.5.1 to 1.7.2 8 days ago which is most likely why none of us could see the minified version served.

I just tested and, indeed, while my normal instance (v2.1.0) is serving a 181k (!!) client, master is correctly serving a 70k client as expected.

Not being minified was the only drawback of using the server-provided version of the client, so I definitely think we should leverage it, at least as long as we are using socket.io on the server.

EDIT:

03:08	+xPaw	Astorije, didn't we agree using serveclient wasn't a good option? The tests is going to need the client, it can't use the serve option
03:20	+xPaw	I also just realised, it's requires as a devdep, installing it twice on npm2 is an absolute minor issue
03:23	+astorije	Yeah we agreed until I realized we've had it minified for 8 days :D For the tests however, yeah, that's true, I overlooked that part. Damn you socket.io
03:24	+astorije	Good point, I kept seeing it in the `dependencies` section so I didn't make the connection. Indeed, much less of an issue.

Webpack

xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Dec 18, 2016

xPaw added this to the 2.2.0 milestone Dec 18, 2016

xPaw requested review from astorije and AlMcKinlay December 18, 2016 15:57

xPaw mentioned this pull request Dec 18, 2016

Webpack all the things #640

Closed

AlMcKinlay approved these changes Dec 18, 2016

View reviewed changes

MaxLeiter mentioned this pull request Dec 18, 2016

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

Merged

xPaw force-pushed the pr/640 branch 2 times, most recently from 649195f to 9d7867c Compare December 21, 2016 14:52

astorije reviewed Dec 22, 2016

View reviewed changes

astorije requested changes Dec 25, 2016

View reviewed changes

astorije approved these changes Dec 26, 2016

View reviewed changes

xPaw force-pushed the pr/640 branch from 444f441 to b280c65 Compare December 26, 2016 15:28

xPaw added 2 commits December 27, 2016 19:15

Webpack

fb87bd3

Remove vendor client code

8fd1d16

xPaw force-pushed the pr/640 branch from b280c65 to 8fd1d16 Compare December 27, 2016 17:15

astorije merged commit e45edff into master Dec 28, 2016

astorije deleted the pr/640 branch December 28, 2016 08:27

matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017

Merge pull request thelounge#817 from thelounge/pr/640

8fb5c4f

Webpack

astorije mentioned this pull request Oct 5, 2017

Moving dependencies to external file so that it is not packed with the code erming/shout#693

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Webpack #817

Webpack #817

xPaw commented Dec 18, 2016

AlMcKinlay left a comment

astorije left a comment

astorije Dec 20, 2016

xPaw Dec 22, 2016

AlMcKinlay Dec 23, 2016

astorije Dec 25, 2016

astorije left a comment

astorije Dec 24, 2016

xPaw Dec 25, 2016

astorije Dec 24, 2016

xPaw Dec 26, 2016

astorije Dec 26, 2016

xPaw Dec 26, 2016

astorije Dec 24, 2016

xPaw Dec 25, 2016

astorije Dec 26, 2016

astorije Dec 24, 2016

xPaw Dec 25, 2016

astorije commented Dec 25, 2016

astorije left a comment

astorije Dec 26, 2016

xPaw Dec 26, 2016

xPaw commented Dec 26, 2016

xPaw commented Dec 26, 2016

astorije commented Dec 26, 2016

xPaw commented Dec 27, 2016

astorije commented Dec 28, 2016 •

edited

Loading

Webpack #817

Webpack #817

Conversation

xPaw commented Dec 18, 2016

AlMcKinlay left a 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

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

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 Dec 25, 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

xPaw commented Dec 26, 2016

xPaw commented Dec 26, 2016

astorije commented Dec 26, 2016

xPaw commented Dec 27, 2016

astorije commented Dec 28, 2016 • edited Loading

astorije commented Dec 28, 2016 •

edited

Loading