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

Remove config.js in favor of .env #92

Merged
merged 3 commits into from
Jul 10, 2017
Merged

Conversation

phyrog
Copy link
Contributor

@phyrog phyrog commented Jul 7, 2017

Makes it easier to inject build time options like the backend host by setting the REACT_APP_BACKEND_HOST environment variable. See ontohub/deployment#9.

@codecov
Copy link

codecov bot commented Jul 7, 2017

Codecov Report

Merging #92 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #92   +/-   ##
=======================================
  Coverage   95.41%   95.41%           
=======================================
  Files          10       10           
  Lines         131      131           
  Branches       18       18           
=======================================
  Hits          125      125           
  Misses          5        5           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c5fda7...b3074ce. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 7, 2017

Codecov Report

Merging #92 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #92   +/-   ##
=======================================
  Coverage   95.41%   95.41%           
=======================================
  Files          10       10           
  Lines         131      131           
  Branches       18       18           
=======================================
  Hits          125      125           
  Misses          5        5           
  Partials        1        1

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c5fda7...3861f29. Read the comment docs.

.env Outdated
@@ -1 +1,4 @@
PORT=3001

REACT_APP_BACKEND_HOST="http://localhost:3000"
REACT_APP_BACKEND_VERSION="> 0.0.0-90"
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that the backend version requirement should be part of the .env file. It is a requirement that the code makes, not one that the server makes. Maybe we should keep a config.js for things like this and have an additional .env file for things like the backend host address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think a good solution would be to specify the backend version in the package.json file (custom fields are allowed), but importing the file (and all other files outside of src) is currently not allowed. There is a PR for that however that seems to be near completion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a work-around for now, I can symlink package.json to src/config.json. Any downsides to symlinks in git?

@eugenk
Copy link
Member

eugenk commented Jul 9, 2017

Since you are fixing the frontend-backend communication here, can you please add a request header Accept: application/json to every request that is made to the backend?

Copy link
Member

@eugenk eugenk left a comment

Choose a reason for hiding this comment

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

@phyrog phyrog merged commit e86072d into master Jul 10, 2017
@phyrog phyrog deleted the move_build_config_to_env_files branch July 12, 2017 09:40
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.

2 participants