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.

Sign up for GitHub

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

Status endpoints #53

Merged
merged 3 commits into from
Apr 1, 2020
Merged

Status endpoints #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3bd3329
Adding env variables and loading support for middleware plugins
schottra Mar 11, 2020
1b7061d
Adds json parsing middleware to ensure that custom POST route handler…
schottra Mar 12, 2020
61cb82c
Also ignoring docker file and dockerignore
schottra Mar 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .dockerignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
.git
.env
node_modules
Dockerfile
.dockerignore
npm-debug.log
.storybook
.vscode
14 changes: 13 additions & 1 deletion env.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,15 @@ const CORS_PROXY_PREFIX = process.env.CORS_PROXY_PREFIX || '/cors_proxy';
const CONFIG_DIR = process.env.CONFIG_DIR || './config';
const CONFIG_CACHE_TTL_SECONDS = process.env.CONFIG_CACHE_TTL_SECONDS || '60';

// Defines a file to be required which will provide implementations for
// any user-definable code.
const PLUGINS_MODULE = process.env.PLUGINS_MODULE;

// Optional URL which will provide information about system status. This is used
// to show informational banners to the user.
// If it has no protocol, it will be treated as relative to window.location.origin
const STATUS_URL = process.env.STATUS_URL;

module.exports = {
ADMIN_API_URL,
BASE_URL,
Expand All @@ -23,10 +32,13 @@ module.exports = {
CONFIG_DIR,
CORS_PROXY_PREFIX,
NODE_ENV,
PLUGINS_MODULE,
STATUS_URL,
processEnv: {
ADMIN_API_URL,
BASE_URL,
CORS_PROXY_PREFIX,
NODE_ENV
NODE_ENV,
STATUS_URL
}
};
7 changes: 7 additions & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,17 +8,24 @@ const fs = require('fs');
const morgan = require('morgan');
const express = require('express');
const env = require('./env');
const { applyMiddleware } = require('./plugins');

const corsProxy = require('./corsProxy.js');

const app = express();

// Enable logging for HTTP access
app.use(morgan('combined'));
app.use(express.json());

app.get(`${env.BASE_URL}/healthz`, (_req, res) => res.status(200).send());
app.use(corsProxy(`${env.BASE_URL}${env.CORS_PROXY_PREFIX}`));

if (typeof applyMiddleware === 'function') {
console.log('Found middleware plugins, applying...');
applyMiddleware(app);
}

if (process.env.NODE_ENV === 'production') {
const path = require('path');
const expressStaticGzip = require('express-static-gzip');
Expand Down
11 changes: 11 additions & 0 deletions plugins.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
const env = require('./env');

const { middleware } = env.PLUGINS_MODULE ? require(env.PLUGINS_MODULE) : {};

if (Array.isArray(middleware)) {
module.exports.applyMiddleware = app => middleware.forEach(m => m(app));
} else if (middleware !== undefined) {
console.log(
`Expected middleware to be of type Array, but got ${middleware} instead`
Copy link

Choose a reason for hiding this comment

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

Won't this case always be hit when there is no env.PLUGINS_MODULE set? is that desired?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps the conditional should be else to handle all the other cases here. That would cover the case where require(env.PLUGINS_MODULE).middleware resolves to undefined. The code as it is written wouldn't export anything, nor log anything to the console.

Copy link

Choose a reason for hiding this comment

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

sorry, 🤦‍♂ you are right. I misread the destructuring on that assignment... disregard

Copy link
Contributor Author

@schottra schottra Mar 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Looking at it now, I realize I may have been a little cryptic with this code. The intended behavior is that if you specify env.PLUGINS_MODULE, and it exports an object with property middleware, that property will be an array.
If it is not an array, or it is undefined, we will ignore it altogether. Plugins/middleware are completely optional and shouldn't interrupt the startup with an error.

The else if case was meant to be informational. In the event that you set the environment variable and your module exports a value, you clearly want to add middleware. So we let you know why we can't use your value.

And in the case where require(env.PLUGINS_MODULE).middleware resolves to undefined, we aren't modifying module.exports. This will result in the module exporting an empty object. To me that seems like the desirable behavior. But it's probably not clear what conventions/behaviors are expected here.

Maybe this could use more documentation and more robust logging in all cases?

);
}
1 change: 1 addition & 0 deletions src/config/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,5 @@ export interface Env extends NodeJS.ProcessEnv {
BASE_URL?: string;
CORS_PROXY_PREFIX: string;
DISABLE_ANALYTICS?: string;
STATUS_URL?: string;
}