CenterForDigitalHumanities / CenterForDigitalHumanities/TPEN-services
Inconsistent optional chaining on startsWith
- Dominant language
- JavaScript
- Stars
- 2
- Forks
- 0
- PR merge metrics
- No merged PRs in 30d
Description
## Description
`startsWith(process.env.RERUMIDPREFIX)` is used across 19 call sites with 3 different optional chaining patterns. This inconsistency makes it unclear whether the defensive chaining is intentional or accidental, and creates confusion about what invariants are expected for `id` values.
## Current Patterns
### Pattern A: `id?.startsWith?.(...)` — double optional chain (2 occurrences)
Guards both a null/undefined `id` AND a non-string `id`.
| File | Line | Context |
|------|------|---------|
| `classes/Layer/Layer.js` | 113 | `asJSON` — hydration guard |
| `classes/Page/Page.js` | 253 | `asJSON` — hydration guard |
### Pattern B: `id.startsWith?.(...)` — method-only optional chain (4 occurrences)
Guards a non-string `id` but will throw if `id` is null/undefined.
| File | Line | Context |
|------|------|---------|
| `classes/Layer/Layer.js` | 148 | `#loadAnnotationCollectionDataFromRerum` — inner guard |
| `classes/Page/Page.js` | 81 | `#loadAnnotationPageDataFromRerum` — inner guard |
| `classes/Line/Line.js` | 23 | Constructor — set `#tinyAction` |
| `classes/Line/Line.js` | 114 | `#loadAnnotationDataFromRerum` — inner guard |
### Pattern C: `id.startsWith(...)` — no optional chain (13 occurrences)
Plain call. Throws if `id` is null/undefined or not a string. This is the most common pattern.
| File | Line | Context |
|------|------|---------|
| `classes/Layer/Layer.js` | 13 | `#setRerumId` — negated guard |
| `classes/Layer/Layer.js` | 48 | Constructor — set `#tinyAction` |
| `classes/Layer/Layer.js` | 92 | `update()` — `pages.some(page => page.id.startsWith(...))` |
| `classes/Page/Page.js` | 13 | `#setRerumId` — negated guard |
| `classes/Page/Page.js` | 34 | Constructor — set `#tinyAction` |
| `classes/Line/Line.js` | 10 | `#setRerumId` — negated guard |
| `classes/Project/ProjectFactory.js` | 194 | `cloneProject` — layer ID check |
| `classes/Project/ProjectFactory.js` | 226 | `clonePagesWithAnnotations` — page ID check |
| `classes/Project/ProjectFactory.js` | 517 | `importTPEN2` — layer ID check |
| `classes/Project/ProjectFactory.js` | 776 | `toManifest` — page ID check |
| `utilities/shared.js` | 89 | `updateLayerAndProject` — filter RERUM pages |
| `utilities/shared.js` | 142 | `updatePageAndProject` — check if page is in RERUM |
| `page/index.js` | 191 | Page route — negated check on old ID |
## Recommendation
Pick one pattern and apply it consistently. Since constructors validate that `id` is truthy and all IDs are strings, Pattern C (`id.startsWith(...)`) is sufficient and is already used in 13 of 19 call sites. The optional chaining in Patterns A and B adds no practical safety given the constructor guards.
Alternatively, if defensive chaining is preferred for resilience against unexpected data from external sources (RERUM), standardize on Pattern A (`id?.startsWith?.(...)`) everywhere.
## Affected Classes
- `classes/Layer/Layer.js`
- `classes/Page/Page.js`
- `classes/Line/Line.js`
- `classes/Project/ProjectFactory.js`
- `utilities/shared.js`
- `page/index.js`
Contributor guide
Research direction
Review the 19 listed startsWith call sites in classes/Layer/Layer.js, classes/Page/Page.js, classes/Line/Line.js, classes/Project/ProjectFactory.js, utilities/shared.js, and page/index.js. Start with the constructor validation and compare the existing patterns; done means one consistent optional-chaining convention is used across all listed locations.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- javascript
- Domain
- backend
- Issue type
- Refactor
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 58/100