Do not uglify builds when running start-dev #858

xPaw · 2016-12-30T11:22:46Z

Tweaks bundle paths (client vs client/js) to not confuse sourcemaps as much. Disables uglifyjs plugin when running with --watch. Also reduces production built file size a little more.

astorije · 2017-01-02T08:39:12Z

webpack.config.js

@@ -2,11 +2,12 @@

 const webpack = require("webpack");
 const path = require("path");
+const isWatch = process.argv.includes("--watch");


Should we be relying on NODE_ENV instead?

astorije · 2017-01-02T08:46:14Z

package.json

-    "build:webpack": "webpack",
-    "watch": "webpack -w",
+    "build:webpack": "webpack -p",
+    "watch": "webpack -d --watch",


For whoever is wondering, node_modules/.bin/webpack -h says:

-d shortcut for --debug --devtool sourcemap --output-pathinfo -p shortcut for --optimize-minimize

I see this as unofficial development/production flags, am I correct?
If so, it's a bit confusing that build:webpack implies production and watch implies development. I know I sometimes use npm run build:webpack when I just want a single build without a FS watch.

IMO (but MO is fairly Webpack-inexperienced 😅), I think we should have the commands be environment-independent, and the configuration files behave differently depending on the environment (or select different files depending on environment, if the configurations are too disjoint).
Is that stupid?

Just for the record:

--devtool sourcemap is already specified in our Webpack config.

--optimize-minimize is the same as the UglifyJsPlugin in our Webpack config.

--debug enables debug mode of loaders, which is something we probably do not want on development env at all time.

If I understand --output-pathinfo, it is of little interest.

astorije · 2017-01-02T23:31:57Z

Playing a bit with the current state, we definitely cannot have configurations differ solely on build:webpack vs. watch. When you check out a branch and run npm install, this (at least until #737 gets addressed) this assumes prod.

I think our best bet is to rely on NODE_ENV to be production (which is also set when using --production).

xPaw · 2017-01-03T17:46:10Z

I wanted to use env at first, but setting it in script and making it crossplatform is not easy.

astorije · 2017-01-03T17:59:36Z

I don't think we have a choice though. Could use https://www.npmjs.com/package/cross-env for cross-platform in scripts?

xPaw · 2017-01-03T18:02:16Z

I think we can do a two liner js to set the environment.

…

On Tue, 3 Jan 2017, 19:59 Jérémie Astori, ***@***.***> wrote: I don't think we have a choice though. Could use https://www.npmjs.com/package/cross-env for cross-platform in scripts? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#858 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAlb07-f74NJIcIwtUt38PIRXuz0MAf4ks5rOoyIgaJpZM4LYIpH> .

astorije · 2017-01-04T00:15:50Z

You mean to emulate cross-env?

Anyway, it doesn't really matter I think, since it's mostly NODE_ENV=production npm install which will be useful. The rest should assume default NODE_ENV or use the one being set.

astorije

Cf. discussions in comment.

leaexplores · 2017-01-05T04:48:52Z

webpack.config.js

 	]
 };
+
+if (!isWatch) {


You could use the node_env directly from here if you wanted to filter it from production and development build.

process.env.NODE_ENV

leaexplores · 2017-01-05T04:49:53Z

Using the process.env.NODE_ENV to determine what to build or not is how most of the webpack-built project does. I left a comment in the code if we want to include the uglify plugin only when in production (NODE_ENV == PRODUCTION)...

astorije · 2017-01-07T23:04:29Z

package.json

-    "build:webpack": "webpack",
-    "watch": "webpack -w",
+    "build:webpack": "webpack -p",
+    "watch": "webpack -d --watch",


Just for the record:

--devtool sourcemap is already specified in our Webpack config.

--optimize-minimize is the same as the UglifyJsPlugin in our Webpack config.

--debug enables debug mode of loaders, which is something we probably do not want on development env at all time.

If I understand --output-pathinfo, it is of little interest.

astorije · 2017-01-08T00:09:03Z

.travis.yml


 cache:
  directories:
    - ~/.npm

+before_script:
+  - NODE_ENV=$BUILD_ENV npm run build
+


This works fine when BUILD_ENV is not defined, which is the case outside of that included combination.

So, if it isn't defined, does it just ignore that, and default to not production?

Correct, if BUILD_ENV isn't defined, it runs NODE_ENV= npm run build which is similar as setting an empty environment (i.e. a not production).

astorije · 2017-01-08T00:19:42Z

Alright, spent a looot of time on this, between reading docs and GitHub threads for Webpack, npm, etc. I believe this PR puts us in an optimal state regarding recent introduction of Webpack.

This PR resolves the first 2 items of #857 (comment).

Posting here my commit message details + extra info:

1. Use Webpack configuration based on NODE_ENV instead of watch/no-watch

Also, move the DedupePlugin to the prod-specific section. Webpack doc itself recommends to not run this outside of production.

~~Note that this currently breaks cross-OS support of npm run build. This will be fixed in a latter commit.~~ This was temporary and on purpose to not mix the unrelated commits of Webpack-for-production and no-npm run build-on-npm install. It's fixed by the next commit.

This commit addresses my comments made in this PR. Non-cross-OS character of this commit is the reason why the next commits are part of this PR.

2. Make and document `npm run build` now a mandatory step of the install/build setup

This has several reasons, benefits and consequences:

When running on root (which is not recommended anyway), npm run build was already necessary.
This allows to not use the prepublish hook, whose behavior is going to change in npm v5 and again in npm v6.
This allows to create both production and development builds when running from source.
It makes npm run build compatible with Windows again for development environments (lost in previous commit).
It uses the prepublishOnly hook added in npm v4. Since this hook is not available prior to that, deployment to npm from Travis has to be done on the Node.js v7 environment.

This fixes #737.

3. Add an extra Travis CI job to make sure production build succeeds and passes tests

This only adds one extra job to the build (which still runs all its jobs concurrently, so build time does not increase) to make sure what we deploy or will is actually correct.

This is highly desirable since we now produce 2 different files in development/testing and production.

4. Add a basic check for bundled application when starting the server

Note that this will not detect if the client application was built with an old version of the repo.

This partially addresses #737 (comment), but a naive check is better than no checks!

xPaw · 2017-01-08T06:53:28Z

package.json

    "test": "npm-run-all -c test:* lint",
    "test:mocha": "mocha",
    "lint": "npm-run-all -c lint:*",
    "lint:js": "eslint .",
    "lint:css": "stylelint \"**/*.css\"",
-    "prepublish": "npm run build"
+    "prepublishOnly": "NODE_ENV=production npm run build"


This won't execute on Windows, fyi.

That should be fine, because we should only ever be publishing from travis.

What @YaManicKill said. You can still run npm run build on Windows, and if someone ever needs to deploy from Windows, which should only happen in case something goes wrong on Travis CI, one can always change prepublishOnly so a Windows-friendly command temporarily.

astorije · 2017-01-08T07:04:00Z

Yeah I know but it's OK as it's only relevant when publishing, which we only do from Travis builds. If comes a time when we don't publish through Travis as a one-off, chances are I'd publish myself (not Windows) and if not, one can tweak to use Windows' `set` for this one-off. All other commands, for day to day development, work on all platforms.

xPaw · 2017-01-08T08:42:39Z

src/server.js

@@ -18,6 +18,11 @@ var authFunction = localAuth;
 module.exports = function() {
 	manager = new ClientManager();

+	if (!fs.existsSync("client/js/bundle.js")) {


Ideas on how we would check hashes to tell the file is out of date?

Solutions I can think of are not ideal. For example, I don't know if this is possible without forcing people committing on the client to include such marker in their PR.

Either way, can we do this in a latter PR please? This PR should not go further than what it currently does.

Yeah, let's look into this in another one. I guess you could do it by the first line of the bundle being a version number or git hash or something.

Also, I would probably move this block into index.js. I'm not sure it belongs in server.

Ok, I'll move it.

So, I've tried to move it to index.js and I'd definitely keep it where it is: index.js doesn't have access to log at the moment (probably not a big deal but still) but way more importantly, it's blocking -h, the list subcommand, etc.

Let's keep it that way for now and we can improve that later.

leaexplores · 2017-01-08T08:57:51Z

webpack.config.js

-		path: path.resolve(__dirname, "client/js"),
-		filename: "bundle.js",
+		path: path.resolve(__dirname, "client"),
+		filename: "[name]",


Usually with webpack, you'll be using the compilation hash that can easily be appended at build.

You would configure webpack to output like so inside the webpack config...

module.exports = { ... output: { path: path.join(__dirname, 'dist'), pathinfo: (env !== 'production'), filename: 'app-[hash:6].js' } ... }

This will output a app-rand6c.js (app- 6 random characters as i've requested 6). filename in the output folder.

From there you could include it easily in the server.js

Also, move the `DedupePlugin` to the prod-specific section. [Webpack doc](https://webpack.github.io/docs/list-of-plugins.html#dedupeplugin) itself recommends to not run this outside of production. Note that this currently breaks cross-OS support of `npm run build`. This will be fixed in a latter commit.

astorije · 2017-01-11T05:17:21Z

@thelounge/maintainers, yes, no, anyone? :)
v2.2.0 is sooo close... !

AlMcKinlay · 2017-01-11T08:39:18Z

@astorije Sorry, didn't realise you guys had stopped messing around with this PR :-P I sort of phased out the notifications. I'll take a look now.

AlMcKinlay

Yeah, looks good. I've not tested this, but assuming 1 of you have.

Are we 100% sure that the bundle is included in the npm publish?

AlMcKinlay · 2017-01-11T08:40:00Z

.travis.yml


 cache:
  directories:
    - ~/.npm

+before_script:
+  - NODE_ENV=$BUILD_ENV npm run build
+


So, if it isn't defined, does it just ignore that, and default to not production?

AlMcKinlay · 2017-01-11T08:42:03Z

package.json

    "test": "npm-run-all -c test:* lint",
    "test:mocha": "mocha",
    "lint": "npm-run-all -c lint:*",
    "lint:js": "eslint .",
    "lint:css": "stylelint \"**/*.css\"",
-    "prepublish": "npm run build"
+    "prepublishOnly": "NODE_ENV=production npm run build"


That should be fine, because we should only ever be publishing from travis.

AlMcKinlay · 2017-01-11T08:54:37Z

src/server.js

@@ -18,6 +18,11 @@ var authFunction = localAuth;
 module.exports = function() {
 	manager = new ClientManager();

+	if (!fs.existsSync("client/js/bundle.js")) {


Yeah, let's look into this in another one. I guess you could do it by the first line of the bundle being a version number or git hash or something.

xPaw · 2017-01-14T08:59:51Z

Fyi, building from git will most likely not minify (as you have to explicitly set node env to prod). The changes are fine for now.

xPaw · 2017-01-14T09:00:42Z

.travis.yml

@@ -6,11 +6,17 @@ node_js:

 matrix:
  fast_finish: true
+  include:
+    - node_js: 7 # Version used to deploy to npm registry
+      env: BUILD_ENV=production



Can't set node env directly?

No, otherwise devDependencies aren't installed. The rationale is: run npm install in non-production environment to be able to build, but build for production environments at release time.

xPaw · 2017-01-14T09:01:58Z

README.md

- While it is the most recent codebase, this is not production-ready!
- It is not recommended to run this as root. However, if you decide to do so,
-  you will have to run `npm run build`.
+⚠️ Run at your own risk. It is also not recommended to run this as root.


Keep the mention that all development happens in master and it may not be stable.

…/build setup This has several reasons, benefits and consequences: - When running on root (which is not recommended anyway), `npm run build` was already necessary. - This allows to not use the `prepublish` hook, whose behavior is going to change in npm v5 and again in npm v6. - This allows to create both production and development builds when running from source. - It makes `npm run build` compatible with Windows again for development environments (lost in previous commit). - It uses the `prepublishOnly` hook added in npm v4. Since this hook is not available prior to that, deployment to npm from Travis has to be done on the Node.js v7 environment.

… passes tests

Note that this will not detect if the client application was built with an old version of the repo.

astorije · 2017-01-23T06:17:01Z

Alright, I believe (well, I hope 😅) this one is finally good to go. @xPaw, are you happy with it?

xPaw · 2017-01-23T10:27:36Z

👍

AlMcKinlay · 2017-01-23T10:42:42Z

Well, I'll merge, then.

Do not uglify builds when running start-dev

xPaw assigned astorije Dec 30, 2016

xPaw added this to the 2.2.0 milestone Dec 30, 2016

astorije reviewed Jan 2, 2017

View reviewed changes

astorije mentioned this pull request Jan 2, 2017

Front-end develop/debug experience is much worse than pre-Webpack era #857

Closed

3 tasks

astorije requested changes Jan 4, 2017

View reviewed changes

astorije added Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. priority and removed priority labels Jan 4, 2017

leaexplores reviewed Jan 5, 2017

View reviewed changes

astorije force-pushed the xpaw/tweak-webpack branch from 7ba8b3a to f34f34e Compare January 7, 2017 23:06

astorije approved these changes Jan 8, 2017

View reviewed changes

astorije mentioned this pull request Jan 8, 2017

Npm Prepublish will be changing #737

Closed

xPaw commented Jan 8, 2017

View reviewed changes

leaexplores reviewed Jan 8, 2017

View reviewed changes

xPaw and others added 2 commits January 10, 2017 13:07

Do not uglify builds when running start-dev

a8dd136

astorije force-pushed the xpaw/tweak-webpack branch from f3dd0fc to c9021dc Compare January 10, 2017 18:07

AlMcKinlay approved these changes Jan 11, 2017

View reviewed changes

xPaw commented Jan 14, 2017

View reviewed changes

astorije added 3 commits January 23, 2017 01:15

Add an extra Travis CI job to make sure production build succeeds and…

c7e8000

… passes tests

Add a basic check for bundled application when starting the server

bc8b699

Note that this will not detect if the client application was built with an old version of the repo.

astorije force-pushed the xpaw/tweak-webpack branch from c9021dc to bc8b699 Compare January 23, 2017 06:16

AlMcKinlay merged commit 91ae814 into master Jan 23, 2017

AlMcKinlay deleted the xpaw/tweak-webpack branch January 23, 2017 10:42

astorije mentioned this pull request Feb 12, 2017

Fix run_pr.sh script #919

Merged

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

Merge pull request thelounge#858 from thelounge/xpaw/tweak-webpack

7fac5c6

Do not uglify builds when running start-dev

Do not uglify builds when running start-dev #858

Do not uglify builds when running start-dev #858

Conversation

xPaw commented Dec 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented Jan 2, 2017

xPaw commented Jan 3, 2017

astorije commented Jan 3, 2017

xPaw commented Jan 3, 2017 via email

astorije commented Jan 4, 2017

astorije left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leaexplores commented Jan 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlMcKinlay Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented Jan 8, 2017

1. Use Webpack configuration based on NODE_ENV instead of watch/no-watch

2. Make and document npm run build now a mandatory step of the install/build setup

3. Add an extra Travis CI job to make sure production build succeeds and passes tests

4. Add a basic check for bundled application when starting the server

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented Jan 8, 2017 via email

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 Jan 11, 2017

AlMcKinlay commented Jan 11, 2017

AlMcKinlay left a comment

Choose a reason for hiding this comment

AlMcKinlay Jan 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xPaw commented Jan 14, 2017

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 Jan 23, 2017

xPaw commented Jan 23, 2017

AlMcKinlay commented Jan 23, 2017

AlMcKinlay Jan 11, 2017 •

edited

Loading

2. Make and document `npm run build` now a mandatory step of the install/build setup

AlMcKinlay Jan 11, 2017 •

edited

Loading