-
Notifications
You must be signed in to change notification settings - Fork 699
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
Changes from all commits
a8dd136
d8f1690
552fa3f
c7e8000
bc8b699
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
cache: | ||
directories: | ||
- ~/.npm | ||
|
||
before_script: | ||
- NODE_ENV=$BUILD_ENV npm run build | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This works fine when There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, if |
||
notifications: | ||
email: | ||
on_success: never | ||
|
@@ -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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This won't execute on Windows, fyi. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What @YaManicKill said. You can still run |
||
}, | ||
"keywords": [ | ||
"lounge", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,6 +18,11 @@ var authFunction = localAuth; | |
module.exports = function() { | ||
manager = new ClientManager(); | ||
|
||
if (!fs.existsSync("client/js/bundle.js")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'll move it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, I've tried to move it to 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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
|
@@ -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]", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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...
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: { | ||
|
@@ -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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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: runnpm install
in non-production environment to be able to build, but build for production environments at release time.