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

Do not uglify builds when running start-dev #858

Merged
merged 5 commits into from
Jan 23, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't set node env directly?

Copy link
Member

Choose a reason for hiding this comment

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

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.

cache:
directories:
- ~/.npm

before_script:
- NODE_ENV=$BUILD_ENV npm run build

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@AlMcKinlay AlMcKinlay Jan 11, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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).

notifications:
email:
on_success: never
Expand All @@ -23,6 +29,7 @@ deploy:
api_key:
secure: I9iN31GWI+Mz0xPw81N7qh1M6uidB+3BmiPUXt8QigX45zwp9EhvfZ0U/AIdUyQwzK2RK1zLRQSt+2/1jyeVi+U+AAsRRmaAUx8iqKaQPAkPnQtElolgRP04WSgo7fvNejfM7zS939bQNKG3RlSm04yPgu+ke2igf799p2bpFe2LtyoEeIiUfrUkBiMSpMguN9XF8a7jqCyIouTKjXHR24RmzJ9r7ZoMV27yQauS7XlD81bontzNRZxTytDKdJpZ+sxGIT9mbbtM4LUFX8MeNe3p/bjWavEhrO0ZIpkbOfS/L/w1375YDoNPXxCs288lnGUH+NbGNAEfn+BTz8cmUp7jI7QWR/kNACPeopdAX4OdZxT8wfQcfQZrfCuSpKciOMC7vGgPpQqjQ61t1RKcKs9VUnwC0SwWjyo8LlzkFKnP1ks0eDGYsSoPLdpC9+76UmePkQdxMhscO8TOgkOCcsTMLiyt6ABGOGKu2iE5SsjUYtPiSiRzSBAQENoO560+xBSVTKwqvvhzUAIt4AuAQSgsFjAylDdyzKoObHX12hBdALrqSOOSVwwIQ5/jTgNAsilURHo7KPD407PhRnLOsvumL0qg4sr9S1hjuUKnNla5dg9GY8FVjJ+b2t0A2vgfG1pR1e3vrJRXrpkfRorhmjvKAk2o5you5pQ1Itty7rM=
on:
node: '4'
node: 7
condition: "$BUILD_ENV = production"
tags: true
repo: thelounge/lounge
8 changes: 3 additions & 5 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,12 @@ The following commands install the development version of The Lounge:
git clone https://github.com/thelounge/lounge.git
cd lounge
npm install
NODE_ENV=production npm run build
npm start
```

A word of caution:

- 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`.
⚠️ While it is the most recent codebase, this is not production-ready! Run at
your own risk. It is also not recommended to run this as root.

## Development setup

Expand Down
1 change: 1 addition & 0 deletions appveyor.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ environment:
install:
- ps: Install-Product node $env:nodejs_version
- appveyor-retry npm install
- npm run build
- npm install mocha-appveyor-reporter
- echo --reporter mocha-appveyor-reporter >> test/mocha.opts

Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@
"build": "npm-run-all build:*",
"build:font-awesome": "node scripts/build-fontawesome.js",
"build:webpack": "webpack",
"watch": "webpack -w",
"watch": "webpack --watch",
"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"
Copy link
Member Author

Choose a reason for hiding this comment

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

This won't execute on Windows, fyi.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

},
"keywords": [
"lounge",
Expand Down
5 changes: 5 additions & 0 deletions src/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ var authFunction = localAuth;
module.exports = function() {
manager = new ClientManager();

if (!fs.existsSync("client/js/bundle.js")) {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I'll move it.

Copy link
Member

Choose a reason for hiding this comment

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

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.

log.error(`The client application was not built. Run ${colors.bold("NODE_ENV=production npm run build")} to resolve this.`);
process.exit();
}

var app = express()
.use(allRequests)
.use(index)
Expand Down
40 changes: 26 additions & 14 deletions webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,14 @@
const webpack = require("webpack");
const path = require("path");

module.exports = {
// ********************
// Common configuration
// ********************

let config = {
entry: {
app: path.resolve(__dirname, "client/js/lounge.js"),
vendor: [
"js/bundle.js": path.resolve(__dirname, "client/js/lounge.js"),
"js/bundle.vendor.js": [
"handlebars/runtime",
"jquery",
"jquery-ui/ui/widgets/sortable",
Expand All @@ -17,8 +21,8 @@ module.exports = {
},
devtool: "source-map",
output: {
path: path.resolve(__dirname, "client/js"),
filename: "bundle.js",
path: path.resolve(__dirname, "client"),
filename: "[name]",

Choose a reason for hiding this comment

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

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

publicPath: "/"
},
module: {
Expand Down Expand Up @@ -53,14 +57,22 @@ module.exports = {
]
},
plugins: [
new webpack.optimize.CommonsChunkPlugin(
"vendor", // chunkName
"bundle.vendor.js" // filename
),
new webpack.optimize.UglifyJsPlugin({
compress: {
warnings: false
}
}),
new webpack.optimize.CommonsChunkPlugin("js/bundle.vendor.js")
]
};

// *********************************
// Production-specific configuration
// *********************************

if (process.env.NODE_ENV === "production") {
config.plugins.push(new webpack.optimize.DedupePlugin());
config.plugins.push(new webpack.optimize.UglifyJsPlugin({
comments: false,
compress: {
warnings: false
}
}));
}

module.exports = config;