Skip to content

Conversation

sizov
Copy link

@sizov sizov commented May 14, 2016

This fixes issues when deploying to Heroku, #41

  • import for ES6 modules is not supported yet natively, but server.js has import statement. This project does not build server.js with any build tool to augument import, so nodejs fails on that import line, had to fix it
  • if you deploy to Heroku from command line, environment variables in app.json are ignored, so you need to set them manually

@coveralls
Copy link

Coverage Status

Changes Unknown when pulling c36e362 on sizov:master into * on pheuter:master*.

@sizov
Copy link
Author

sizov commented May 14, 2016

@coveralls looks fine here, I've tried to git clone original repo from scratch, then add my remote as upstream and was able to fetch and merge changes from my repo into this.

@@ -1,4 +1,4 @@
import express from 'express';
var express = require('express');
Copy link
Owner

Choose a reason for hiding this comment

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

This shouldn't be necessary since the top-level start.js file registers the Babel hook before requireing server.

Copy link
Author

@sizov sizov May 15, 2016

Choose a reason for hiding this comment

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

@pheuter thank you for reply, I understand the idea, but unfortunately without this change I was not able to run this on Heroku (and looks like I'm not the only one).

Please see your original app deployed with Heroku button here: https://original-essential-react.herokuapp.com/ (gives error)
And here is the same deployment to Heroku with button from my fork (1 line changed): https://fix-import-essential-react.herokuapp.com/ (works as expected)

Copy link
Owner

Choose a reason for hiding this comment

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

Hm, I feel hesitant merging this change in just because Heroku isn't properly configured to run the start.js script before running server.jsx, I think figuring that out would be a better change since it wouldn't require modifying application code.

@mdoerneman
Copy link

@sizov Thank you! This was extremely helpful to get things working on Heroku.

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.

4 participants