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

Implement server-side code hot-swapping on file change #1225

Merged
merged 3 commits into from
Aug 15, 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
2 changes: 1 addition & 1 deletion api/index.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,4 @@ app.use notFound
app.use errorHandler

# Start the test server if run directly
app.listen(5000, -> debug "Listening on 5000") if module is require.main
app.listen(5000, -> debug "Listening on 5000") if module is require.main
40 changes: 29 additions & 11 deletions boot.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,10 @@

import artsyXapp from 'artsy-xapp'
import express from 'express'
import { IpFilter } from 'express-ipfilter'
import newrelic from 'artsy-newrelic'
import path from 'path'
import { IpFilter } from 'express-ipfilter'
import { createReloadable, isDevelopment } from 'lib/reloadable'

const debug = require('debug')('app')
const app = module.exports = express()
Expand All @@ -16,23 +18,32 @@ app.use(
)

// Get an xapp token
artsyXapp.init({
const xappConfig = {
url: process.env.ARTSY_URL,
id: process.env.ARTSY_ID,
secret: process.env.ARTSY_SECRET
}, () => {
}

artsyXapp.init(xappConfig, () => {
app.use(newrelic)

// Put client/api together
app.use('/api', require('./api'))
if (isDevelopment) {
const reloadAndMount = createReloadable(app)

// TODO: Possibly a terrible hack to not share `req.user` between both.
app.use((req, rest, next) => {
req.user = null
next()
})
// Enable server-side code hot-swapping on change
app.use('/api', reloadAndMount(path.resolve(__dirname, 'api'), {
mountPoint: '/api'
}))
Copy link
Contributor

Choose a reason for hiding this comment

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

👍


app.use(require('./client'))
invalidateUserMiddleware(app)
reloadAndMount(path.resolve(__dirname, 'client'))

// Staging, Prod
} else {
app.use('/api', require('./api'))
invalidateUserMiddleware(app)
app.use(require('./client'))
}

// Start the server and send a message to IPC for the integration test
// helper to hook into.
Expand All @@ -50,3 +61,10 @@ artsyXapp.on('error', (error) => {
console.warn(error)
process.exit(1)
})

const invalidateUserMiddleware = (app) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

app.use((req, rest, next) => {
req.user = null
next()
})
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right this comment can probably be removed since it's pretty old and we actually benefit from not sharing req.user because it keeps our API stateless.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah, gotta fix this

2 changes: 1 addition & 1 deletion client/apps/articles_list/routes.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,4 @@ renderArticles = (res, req, result, published) ->
res.locals.sd.HAS_PUBLISHED = published
res.render 'index',
articles: result.articles || []
current_channel: req.user?.get('current_channel')
current_channel: req.user?.get('current_channel')
2 changes: 1 addition & 1 deletion client/apps/edit/routes.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,4 @@ render = (req, res, article) ->
setChannelIndexable = (channel, article) ->
noIndex = sd.NO_INDEX_CHANNELS.split '|'
if noIndex.includes channel.get('id')
article.set 'indexable', false
article.set 'indexable', false
2 changes: 1 addition & 1 deletion client/index.coffee
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@ setup = require "./lib/setup"
express = require "express"

app = module.exports = express()
setup app
setup app
80 changes: 80 additions & 0 deletions lib/reloadable.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
/**
* Dev utility for server-side code reloading without the restart. In development,
* watch for file-system changes and clear cached modules when a change occurs,
* thus effectively reloading the entire app on request.
*/

export const isDevelopment = process.env.NODE_ENV === 'development'

export function createReloadable (app) {
return (folderPath, options = {}) => {
if (isDevelopment) {
const {
mountPoint = '/'
} = options

const onReload = (req, res, next) => require(folderPath)(req, res, next)

if (typeof onReload !== 'function') {
throw new Error(
'(lib/reloadable.js) Error initializing reloadable: `onReload` is ' +
'undefined. Did you forget to pass a callback function?'
)
}

const watcher = require('chokidar').watch(folderPath)

watcher.on('ready', () => {
watcher.on('all', () => {
Object.keys(require.cache).forEach(id => {
if (id.startsWith(folderPath)) {
delete require.cache[id]
}
})
})
})

let currentResponse = null
let currentNext = null

app.use((req, res, next) => {
currentResponse = res
currentNext = next

res.on('finish', () => {
currentResponse = null
currentNext = null
})

next()
})

/**
* In case of an uncaught exception show it to the user and proceed, rather
* than exiting the process.
*/
process.on('uncaughtException', (error) => {
if (currentResponse) {
currentNext(error)
currentResponse = null
currentNext = null
} else {
process.abort()
}
})

app.use(mountPoint, (req, res, next) => {
onReload(req, res, next)
})

return onReload

// Node env not 'development', exit
} else {
throw new Error(
'(lib/reloadable.js) NODE_ENV must be set to "development" to use ' +
'reloadable.js'
)
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Love this! Very clear and thoughtful error handling

2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
"babel-preset-react": "^6.24.1",
"babel-preset-stage-3": "^6.24.1",
"babelify": "^7.3.0",
"babel-runtime": "^6.25.0",
"backbone": "^1.2.3",
"backbone-super-sync": "^1.1.1",
"bcryptjs": "^2.4.3",
Expand Down Expand Up @@ -101,6 +102,7 @@
"browserify": "^14.1.0",
"browserify-dev-middleware": "^1.5.0",
"caching-coffeeify": "^0.5.1",
"chokidar": "^1.7.0",
"cssify": "^0.7.0",
"eslint": "^3.19.0",
"eslint-config-standard": "^10.2.1",
Expand Down
7 changes: 7 additions & 0 deletions test-reload/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import express from 'express'

const app = module.exports = express()

app.get('/bar', (req, res, next) => {
res.send('working! 41')
})
6 changes: 3 additions & 3 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1956,9 +1956,9 @@ cheerio@^0.19.0:
htmlparser2 "~3.8.1"
lodash "^3.2.0"

chokidar@^1.0.0, chokidar@^1.0.1, chokidar@^1.4.3, chokidar@^1.6.1:
version "1.6.1"
resolved "https://registry.yarnpkg.com/chokidar/-/chokidar-1.6.1.tgz#2f4447ab5e96e50fb3d789fd90d4c72e0e4c70c2"
chokidar@^1.0.0, chokidar@^1.0.1, chokidar@^1.4.3, chokidar@^1.6.1, chokidar@^1.7.0:
version "1.7.0"
resolved "https://registry.yarnpkg.com/chokidar/-/chokidar-1.7.0.tgz#798e689778151c8076b4b360e5edd28cda2bb468"
dependencies:
anymatch "^1.3.0"
async-each "^1.0.0"
Expand Down