Skip to content

Commit

Permalink
feat(config): port defaults to PORT environment variable
Browse files Browse the repository at this point in the history
  • Loading branch information
@ZauberNerd
ZauberNerd committed Apr 4, 2018
1 parent 62b9b77 commit 4a079b8
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 2 deletions.
9 changes: 8 additions & 1 deletion packages/config/__tests__/config.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ describe('config', function() {
expect(hopsConfig).toEqual({
https: false,
host: '0.0.0.0',
port: 8080,
port: undefined,
locations: [],
basePath: '',
assetPath: '',
Expand All @@ -33,6 +33,13 @@ describe('config', function() {
});
});

it('should default port to process.env.PORT', function() {
process.env.PORT = 8091;
var hopsConfig = require('..');
expect(hopsConfig.port).toBe(8091);
delete process.env.PORT;
});

it('should override defaults with config from package.json', function() {
process.chdir(path.join(__dirname, 'fixtures', 'package-json'));
var hopsConfig = require('..');
Expand Down
2 changes: 1 addition & 1 deletion packages/config/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ function applyDefaultConfig(config) {
{
https: false,
host: '0.0.0.0',
port: 8080,
port: parseInt(process.env.PORT, 10) || undefined,
locations: [],
basePath: '',
assetPath: '',
Expand Down

3 comments on commit 4a079b8

@Xiphe
Copy link

@Xiphe Xiphe commented on 4a079b8 Jun 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is a breaking change. Proposing to use parseInt(process.env.PORT, 10) || 8080

@ZauberNerd
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Xiphe check out https://github.com/xing/hops/blob/master/packages/express/lib/utils.js#L83-L86
When hopsConfig.port is defined, we will use that port and exit if it isn't free.
When hopsConfig.port is not defined, we will default to 8080 and increase by one until we find a free port.
So I'd argue that this isn't a breaking change, because the default behavior is basically the same:
When you do not define a port in your config it will use 8080 - previously it would exit if that port wasn't available, now it will just try to find the next free port.

@Xiphe
Copy link

@Xiphe Xiphe commented on 4a079b8 Jun 8, 2018

Choose a reason for hiding this comment

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

We were using import { port } from 'hops-config' to configure our end-to-end tests in development. Previously this was 8080 and now it's undefined. What you're describing means that you internally cared for the breaking change. Nevertheless this change requires a migration for consumers of the package.

But I understand now that defaulting to undefined is cleaner than using the proposed parseInt(process.env.PORT, 10) || 8080. Since it reflects the fact that hops-config does not really know the port.

Please sign in to comment.