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

Build on Deploy #216

Closed
wants to merge 3 commits into from
Closed

Conversation

mnoble
Copy link
Contributor

@mnoble mnoble commented Apr 1, 2016

Moves all build deps to dependencies (opposed to devDependencies) so that the project can be built when deploying to production (like on Heroku, in this case). You enable this by setting the POST_INSTALL_BUILD.

Also fixes an issue introduced after splitting up the backend/static express apps where allowInsecureHTTP was being referenced while never being defined.

@facebook-github-bot
Copy link

By analyzing the blame information on this pull request, we identified @drew-gross, @flovilmart and @hallucinogen to be potential reviewers.

@flovilmart
Copy link
Contributor

@mnoble I've fixed it with #217

"webpack": "~1.12.0"
},
"devDependencies": {
"history": "~1.9.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

history should not be dev I believe

@flovilmart
Copy link
Contributor

While this looks really good, that makes all those dependencies get installed while you use NPM, which is something we obviously want to avoid as we distribute the package with solely the right folders.

also, npm would fetch the devDependencies if NPM_CONFIG_PRODUCTION=false. I believe heroku sets NPM_CONFIG_PRODUCTION=true by default, removing it, would let all devDependencies to be downloaded, therefore you would be able to build on deploy, without impacting the uses using npm

"pig": "http-server ./PIG -p 4041 -s & webpack --config webpack/PIG.config.js --progress --watch",
"build": "NODE_ENV=production webpack --config webpack/production.config.js && webpack --config webpack/PIG.config.js",
"test": "NODE_PATH=./node_modules jest",
"generate": "node scripts/generate.js",
"prepublish": "webpack --config webpack/publish.config.js --progress",
"postinstall": "if [ -n \"$POST_INSTALL_BUILD\" ]; then npm run dist; fi",
Copy link
Contributor

Choose a reason for hiding this comment

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

you could use: "heroku-postbuild" (as described here)

@mnoble
Copy link
Contributor Author

mnoble commented Apr 1, 2016

@flovilmart Yea, some of those deps might just be copy/pasta errors.

So is the general approach to not specify a NODE_ENV, but explicitly specify it in start script or something? That way build has access to all deps+devDeps, while the application runs in production mode? Not having the env set in production seems like an anti-pattern to me, but I'm also not necessarily a Node developer, so ¯_(ツ)_/¯

@flovilmart
Copy link
Contributor

I was mistaken about NODE_ENV, this is actually NPM_CONFIG_PRODUCTION=false ,NODE_ENV will still be production in that case.

to sum up:

  1. for heroku, set heroku config:set NPM_CONFIG_PRODUCTION=false, this will fetch the dev dependencies, but has no other side effects
  2. in package.json add: "heroku-postbuild": "npm run dist" in "scripts"

And that's it'!

@mnoble
Copy link
Contributor Author

mnoble commented Apr 1, 2016

Ahh, gotcha. Heh, I actually didn't even realize Hunter added the heroku-postbuild stuff to the buildpack. Thanks! 👍

@flovilmart
Copy link
Contributor

testing right now but that seem fine

the post build may not even be needed as prepublish will be run after npm install by npm itself and bundles the package by itself

remote:        NPM_CONFIG_LOGLEVEL=error
remote:        NPM_CONFIG_PRODUCTION=false
remote:        NODE_ENV=production
remote:        NODE_MODULES_CACHE=true

So that sums it up by usr setting NPM_CONFIG_PRODUCTION=false

@mnoble
Copy link
Contributor Author

mnoble commented Apr 1, 2016

Yea, the problem was that it compiled everything, but didn't move/symlink it to Parse-Dashboard/public/bundles, which is why I added the dist script to do so. Also, committing compiled production assets to the repo seems less than ideal. Actually, I guess prepublish would run when deploying too.

@flovilmart
Copy link
Contributor

yes prepublish runs, now I have a trouble with heroku, will keep you posted

@flovilmart
Copy link
Contributor

Ok, so it's all great, but we need to remove --progress in prepublish so it don't break on heroku.

@flovilmart
Copy link
Contributor

@mnoble I've remove the --progress that was making heroku build fail. So now the only thing to do I believe is to set the heroku env and you're good to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants