Skip to content

Document optional environment variables - #395

Open
AnasAA98 wants to merge 1 commit into
nytimes:mainfrom
AnasAA98:docs/document-optional-env-vars
Open

AnasAA98 wants to merge 1 commit into
nytimes:mainfrom
AnasAA98:docs/document-optional-env-vars

Conversation

@AnasAA98

Copy link
Copy Markdown

Description of Change

Documents four places where the README doesn't match the actual behavior
of the repo.

  1. npm run build is required before npm test. The error-page
    tests assert that styles are inlined, which needs the compiled
    public/css/errors.css produced by npm run build:inline. CI runs
    npm ci → npm run build → npm test, but the README's Tests
    section only mentions npm test. On a fresh clone that means
    test/functional/pages.test.js fails with no explanation of why.

  2. The Node version note was out of date. It said "Node v8 or
    higher." package.json sets engines.node to >=18.x, and the repo
    also pins v24.4.1 in .nvmrc, which the README never mentioned.
    Now points at .nvmrc instead of a hardcoded number so it can't
    drift again.

  3. Eleven environment variables read by the server were
    undocumented.
    Added a Configuration table covering PORT,
    TRUST_PROXY, PATH_PREFIX, DRIVE_TIMEOUT_SECONDS,
    LIST_UPDATE_DELAY, EDIT_CACHE_DELAY, ALLOW_INLINE_CODE,
    REDIRECT_URL, TEST_EMAIL, GOOGLE_APPLICATION_CREDENTIALS, and
    LOG_LEVEL, with defaults read from the source.

  4. Corrected the drive refresh interval. The App structure section
    said the tree is repopulated "on an interval (currently 60s)."
    server/list.js uses LIST_UPDATE_DELAY || 15, and the comment
    directly above it says 15s. Updated to match and to name the
    environment variable.

The new section was added to the doctoc TOC by hand. I didn't re-run
doctoc, because the current version also rewrites the "Using Docker"
entry (the badge image in that heading confuses it) and drops a blank
line, which would have added unrelated churn to this diff.

Related Issue

None. The LOG_LEVEL row notes that the value is currently ignored and
links #379, which already fixes it.

Motivation and Context

Reproduced on a clean clone at 0c63f81, Node v24.4.1 (matching
.nvmrc), macOS.

Following the README exactly (npm install --no-optional, then
npm test):

FAIL test/functional/pages.test.js
  ● Server responses › that return HTML › should render an inline
    <style> tag and no JS on error pages

    expected '<!DOCTYPE html>…' to match /<style type="text\/css">[^<]/i
    at test/functional/pages.test.js:137:31

Test Suites: 1 failed, 1 skipped, 12 passed, 13 of 14 total
Tests:       1 failed, 3 skipped, 114 passed, 118 total

After npm run build:

Test Suites: 1 skipped, 13 passed, 13 of 14 total
Tests:       3 skipped, 115 passed, 118 total

(The 1 skipped suite is the intentionally disabled
test/functional/playlists.test.js, unrelated to this change.)

Checklist

  • Ran npm run lint and updated code style accordingly
  • npm run test passes
  • PR has a description and all contributors/stakeholder are noted/cc'ed
  • tests are updated and/or added to cover new code — documentation only, no code changes
  • relevant documentation is changed and/or added

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.

1 participant