Open harlantwood opened 9 years ago
This also adds a bin/deploy
script and npm run deploy
command, which can be used to deploy from a local box. Note, however that this will add a commit of the generated assets to your current branch, so CI deploys should be preferred IMO.
A few things from this PR should probably land in the README or a deployment page on the wiki.
BTW, although I believe the deploy to heroku is working just fine, there are issues with my test app on Heroku, possibly due to missing config variables. Currently:
2015-05-19T00:34:19.511899+00:00 app[web.1]: /app/node_modules/express.io/node_modules/express/node_modules/connect/lib/proto.js:103
2015-05-19T00:34:19.511906+00:00 app[web.1]: , search = 1 + req.url.indexOf('?')
2015-05-19T00:34:19.511909+00:00 app[web.1]: ^
2015-05-19T00:34:19.511911+00:00 app[web.1]: TypeError: Cannot read property 'indexOf' of undefined
2015-05-19T00:34:19.511913+00:00 app[web.1]: at Function.app.handle (/app/node_modules/express.io/node_modules/express/node_modules/connect/lib/proto.js:103:27)
2015-05-19T00:34:19.511915+00:00 app[web.1]: at app (/app/node_modules/express.io/node_modules/express/node_modules/connect/lib/connect.js:65:37)
2015-05-19T00:34:19.511916+00:00 app[web.1]: at Object.<anonymous> (/app/app.coffee:66:19)
2015-05-19T00:34:19.511918+00:00 app[web.1]: at Object.<anonymous> (/app/app.coffee:97:4)
2015-05-19T00:34:19.511920+00:00 app[web.1]: at Module._compile (module.js:460:26)
2015-05-19T00:34:19.511921+00:00 app[web.1]: at Object.loadFile (/app/node_modules/coffee-script/lib/coffee-script/coffee-script.js:23:19)
2015-05-19T00:34:19.511923+00:00 app[web.1]: at Module.load (module.js:355:32)
2015-05-19T00:34:19.511925+00:00 app[web.1]: at Function.Module._load (module.js:310:12)
2015-05-19T00:34:19.511926+00:00 app[web.1]: at Module.require (module.js:365:17)
2015-05-19T00:34:19.511928+00:00 app[web.1]: at require (module.js:384:17)
@willzeng (or others) can you post a list of the heroku config
variable names? I don't need their values, (eg DB credentials), just to know what should be configured.
@willzeng (or others) can you post a list of the heroku config variable names? I don't need their values, (eg DB credentials), just to know what should be configured.
Is this what you need?
GRAPHENEDB_URL MONGOHQ_URL PAPERTRAIL_API_TOKEN REDISCLOUD_URL TWITTER_CB_URL TWITTER_CONSUMER_KEY TWITTER_CONSUMER_SECRET
Yes, perfect, thanks @davidfurlong
The PAPERTRAIL one shouldn't be necessary IIRC
@willzeng when you have a chance, try out this PR on your test heroku app...
Just as a #TODO note we should probably make it so that if you deploy to heroku without the correct config variables set then it prompts you for them (or at least shows a useful error before crashing).
So I also thought I set up the config variable correctly on the test heroku, but it is crashing with the same error you had before @harlantwood.
May 19 11:04:35 graphdocs-test app/web.1: /app/node_modules/express.io/node_modules/express/node_modules/connect/lib/proto.js:103
May 19 11:04:35 graphdocs-test app/web.1: , search = 1 + req.url.indexOf('?')
May 19 11:04:35 graphdocs-test app/web.1: ^
May 19 11:04:35 graphdocs-test app/web.1: TypeError: Cannot read property 'indexOf' of undefined
What was the change to fix?
Agreed, I like friendly error messages, I'd be happy with just raising the right error, as anyone deploying should be able to check the logs, eg:
GRAPHENEDB_URL = process.env.GRAPHENEDB_URL or throw new Error "Please set environment variable GRAPHENEDB_URL"
I haven't found the fix for the error we both hit yet @willzeng... Not sure where the error is originating from, as I think the coffee line numbers in my stack trace are bogus (likely from the compiled JS). Can you see the source of the error?
I did npm shrinkwrap
and my deploy / app on heroku works -- see also https://docs.npmjs.com/cli/shrinkwrap
We could either go this way, or figure out what is different this way, and get more specific in package.json
Sweet! I tried upgrading to coffee ~1.9.2 and the errors went away; the app now deploys successfully for me.
I've added this to the pull request.
@willzeng can you confirm that this branch now deploys and the app starts successfully for you, and seems to work well?
App is working locally and starts when depoyed to the test. Right now it seems to fail to find the .css files on the heroku deploy.
GET http://graphdocs-test.herokuapp.com/assets/libs/iconset/flaticon.css
graphdocs-test.herokuapp.com/:1 GET http://graphdocs-test.herokuapp.com/assets/libs/jquery/dist/jquery.min.js
graphdocs-test.herokuapp.com/:1 GET http://graphdocs-test.herokuapp.com/assets/libs/bootstrap/dist/js/bootstrap.min.js
graphdocs-test.herokuapp.com/:1 GET http://graphdocs-test.herokuapp.com/assets/libs/bootstrap/dist/css/bootstrap.min.css
Will take a look at why now.
Right so this was because I forgot to switch over the pages to load from the newly built css and js files. If we do that though, things don't exactly line up so well. Here's the result of trying out main-built.css
and main-built.js
on index.jade
:
Looks like the minifying is affecting the styling somewhere.
Looks like the minifying is effecting the styling somewhere.
I can look at it, but its most likely the ordering of the css rules that it's affecting.. additionally not all pages include all the css files. Ideally we would have separate css files, each minified. Otherwise I can make the different stylesheets compatible with one another, so that minifying and including the lot on each page works.
What would seem to make the most sense is to have one minified .css file for the pages (login, logout, index, signup, userpage etc.) and another minified .css file for the actually application. graph.jade
would then load both the pages.css file and the application.css file, ideally. What do you think?
Note that at present I think we just intended the main-built.css to go with graph.jade
.
What would seem to make the most sense is to have one minified .css file for the pages (login, logout, index, signup, userpage etc.) and another minified .css file for the actually application. graph.jade would then load both the pages.css file and the application.css file, ideally. What do you think?
Note that at present I think we just intended the main-built.css to go with graph.jade.
Ideally (performance wise) we compile a minified version for each page. S.t. each page only loads 1 css file and only has rules which apply to that page
If you want to do it that way then go ahead. Just having the two files seems like it would make code maintenance easier; you're choice though, so go with what you'd like.
On Thu, May 21, 2015 at 1:02 PM David Furlong notifications@github.com wrote:
What would seem to make the most sense is to have one minified .css file for the pages (login, logout, index, signup, userpage etc.) and another minified .css file for the actually application. graph.jade would then load both the pages.css file and the application.css file, ideally. What do you think?
Note that at present I think we just intended the main-built.css to go with graph.jade.
Ideally (performance wise) we compile a minified version for each page. S.t. each page only loads 1 css file and only has rules which apply to that page
— Reply to this email directly or view it on GitHub https://github.com/willzeng/cograph/pull/642#issuecomment-104246195.
If you want to do it that way then go ahead. Just having the two files seems like it would make code maintenance easier; you're choice though, so go with what you'd like.
Option 1:
Option 2:
As for performance differences, a) both solutions have the same # of HTTP requests b) Option 2 will have smaller # of rules in minified stylesheets c) In Option 1, stylesheet HTTP request will cache across pages, whilst Option 2 wont d) The cost (in time) of HTTP requests is greater than the css drawing time for small stylesheets
I think Option 1 may be preferable for the size of the stylesheets we currently have (for performance and convenience)
Agreed. Have given this its own issue for discussion https://github.com/willzeng/cograph/issues/644 We'll want to resolve this before merging this PR.
Hm, do you still want to make this happen @willzeng or @davidfurlong?
Hm, do you still want to make this happen @willzeng or @davidfurlong? Merge pull request
I'm working on the css bit in a branch, I can try and get it done today
Awesome! Just checking in.
+1
On Fri, Jun 5, 2015, 16:13 Harlan T Wood notifications@github.com wrote:
Awesome! Just checking in.
— Reply to this email directly or view it on GitHub https://github.com/willzeng/cograph/pull/642#issuecomment-109324010.
see #647 apologies for the delay
Awesome :)
Can this be merged? @davidfurlong @willzeng
This PR pushes off the app compilation to a CI server, in this case Circle CI (free for open source projects). When you push to a branch of your choice, your code will be deployed to your heroku app. Later, when we add tests to CI, it will only deploy if tests pass.
To use:
production
ordeploy
)HEROKU_APP
, with the value of your Heroku app name to deploy toNow when you push to your branch, CI will:
bin/compile
, precompiling assetsNote that this commit of compiled assets only exists on CI and on Heroku, not on a dev machine, and not on github. This keeps git history free of the noise of generated commits.
As an example, here is Circle CI running this code, and deploying any changes to the
production
branch to my test cograph heroku instance: -- expand thebin/deploy
section to see the compilation and deploy to heroku.Note that this PR obviates #639.