Skip to content

Build and serve client assets from the server container#433

Open
dom-notion wants to merge 2 commits into
mainfrom
dom--bundle-assets-in-dockerfile
Open

Build and serve client assets from the server container#433
dom-notion wants to merge 2 commits into
mainfrom
dom--bundle-assets-in-dockerfile

Conversation

@dom-notion
Copy link
Copy Markdown
Contributor

Context & Requests for Reviewers

This adds client assets to the server container which makes it easier to deploy coop on ECS or something similar without needing a CDN.

Tests

Deployed coop to ecs with these changes. Also verified that the app works locally.

Comment thread server/server.ts
Comment on lines +60 to +65
app.get('{*path}', (_req, res, next) => {
const indexPath = path.join(clientBuildPath, 'index.html');
res.sendFile(indexPath, (err) => {
if (err) next();
});
});
Copy link
Copy Markdown
Member

@juanmrad juanmrad left a comment

Choose a reason for hiding this comment

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

There are several issues with this pr.

helmet, cors, express.json, sessions, passport, and the 404/error handlers all live inside makeApiServer (server/api.ts).

Roughly, I'd want server/server.ts to end up something like:

const ONE_YEAR_SECONDS = 60 * 60 * 24 * 365;

export default async function makeWWWServer(deps: Dependencies) {
  const { app: apiApp, shutdown } = await makeApiApp(deps);
  const app = express();
  app.set('trust proxy', true);
  app.use((req, _res, next) => {
    Object.defineProperty(req, 'secure', { get: () => true });
    Object.defineProperty(req, 'protocol', { get: () => 'https' });
    req.headers['x-forwarded-proto'] = 'https';
    next();
  });
  // Security headers must cover both the API and any static HTML/JS we serve.
  app.use(helmet({ /* production CSP here */ }));
  app.use('/api/v1', apiApp);
  const clientBuildPath =
    process.env.CLIENT_BUILD_PATH ??
    path.resolve(path.dirname(fileURLToPath(import.meta.url)), 'client-build');
  if (existsSync(path.join(clientBuildPath, 'index.html'))) {
    app.use(
      express.static(clientBuildPath, {
        setHeaders(res, filePath) {
          res.setHeader(
            'Cache-Control',
            filePath.endsWith('index.html')
              ? 'no-cache'
              : `public, max-age=${ONE_YEAR_SECONDS}, immutable`,
          );
        },
      }),
    );
    app.get('/{*path}', (req, res, next) => {
      // Only return the SPA shell for navigation requests; let asset misses 404.
      if (path.extname(req.path) !== '' && req.path !== '/') return next();
      if (!req.accepts('html')) return next();
      res.setHeader('Cache-Control', 'no-cache');
      res.sendFile(path.join(clientBuildPath, 'index.html'), (err) => {
        if (err) next();
      });
    });
  }
  return { app, shutdown };
}

Comment thread server/server.ts
// and it's not clear if appending is actually kosher/universally supported.
req.headers['x-forwarded-proto'] = 'https';
next();
});
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should move the helmet set-up here.

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