📝 capture PR #102 learnings in CONVENTIONS.md and DEVELOPMENT.md#108
Open
Valpertui wants to merge 2 commits into
Open
📝 capture PR #102 learnings in CONVENTIONS.md and DEVELOPMENT.md#108Valpertui wants to merge 2 commits into
Valpertui wants to merge 2 commits into
Conversation
CONVENTIONS.md: - Public API JSDoc rules (contract only, no impl details — README is the place for usage walkthroughs). - Deprecation alias pattern (warn-once + JSDoc tag + delegate). - "Public API checklist" — every new export needs index.ts + impl + README + playground + e2e harness + dist rebuild before downstream typecheck. - Defensive validation pattern: validators take `unknown` and use `isValidString` / `isIndexableObject` to defend against JS callers that bypass the TS types. - Derive types from generated schemas, don't duplicate enum literals. - displayError vs displayWarn split (drop-vs-emit semantics). DEVELOPMENT.md: - Auto-generated files table — CHANGELOG.md (generated by `yarn release`) joins src/rumEvent.types.ts on the "do not edit by hand" list.
bcaudan
reviewed
Apr 29, 2026
Comment on lines
+26
to
+35
| ## Public API Checklist | ||
|
|
||
| When adding or renaming a top-level export from `src/index.ts`, every change below MUST land in the same PR: | ||
|
|
||
| - [ ] `src/index.ts` — export with full JSDoc (contract + `@experimental`/`@deprecated` if applicable). | ||
| - [ ] Domain implementation (`src/domain/.../*Collection.ts`) — actual behavior + unit tests. | ||
| - [ ] **`README.md`** — usage example under the matching `## API` subsection. Public docs live here; don't put walkthroughs in JSDoc. | ||
| - [ ] **Playground** — IPC handler + button so the new API is exercisable locally without writing a one-off Electron app. | ||
| - [ ] **e2e harness** (`e2e/app/src/main.ts`, `e2e/app/src/preload.ts`, `e2e/lib/mainPage.ts`) + at least one scenario in `e2e/scenarios/` exercising the API end-to-end against the local intake mock. | ||
| - [ ] If the public surface changes, **rebuild `dist/`** before typechecking the playground / e2e: they consume `@datadog/electron-sdk` via portal/yarn workspace and read from `dist/index.d.ts`. Stale dist is the most common cause of "Module has no exported member 'X'" after a rename. |
Collaborator
There was a problem hiding this comment.
💬 suggestion: as it is more a workflow than a rule, it would probably be better in DEVELOPMENT.md
| | File | Generated by | Editing it instead | | ||
| | ----------------------- | --------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------- | | ||
| | `CHANGELOG.md` | `yarn release` (see "Releasing" below) — sections built from commit subjects since the previous tag | Edit commit messages or the editor-prompt during the release; the changelog rebuilds from there | | ||
| | `src/rumEvent.types.ts` | `yarn json-schemas:generate` from `rum-events-format` schemas | Update the upstream schema in `rum-events-format`, then regenerate | |
Collaborator
There was a problem hiding this comment.
💬 suggestion: we already cover schema management in the previous section, maybe we could link the sections instead of duplicating the content
Comment on lines
3
to
+7
| ## Code Documentation | ||
|
|
||
| Update relevant code documentation (JSDoc comments, inline comments) when modifying function behavior. Keep documentation in sync with implementation. | ||
|
|
||
| ### Public API JSDoc |
Collaborator
There was a problem hiding this comment.
💬 suggestion: alternate structure
## Documentation Maintenance
[existing 1-liner: keep code documentation in sync with implementation]
## Public API
(all that is related to public API)
[intro: applies to src/index.ts exports]
### JSDoc
### Deprecated Aliases
### Defensive Input Validation
...
## Logging Severity
(we may want to enrich it with other severity)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
PR #102 (the main-process RUM Operations APIs) surfaced a handful of recurring patterns that aren't yet written down anywhere in the repo. Capturing them now means the next contributor (human or agent) doesn't rediscover the same notes through code review.
This is a docs-only change. No behavior change, no API surface change.
Changes
docs/CONVENTIONS.md— five new subsections:src/index.tsexports carries the contract, not implementation reasoning. Cross-link to the README "feature" section for usage walkthroughs (@see README "Operation Monitoring"is the precedent we just established).@deprecatedaliases — when renaming a public API after it has shipped, keep the old name as a deprecated alias that warns once per method and forwards to the new one. Drop in the next major.dist/rebuild before downstream typecheck). The dist-rebuild step is the easy one to forget — it's the most common cause of "Module has no exported member" after a public-API rename.unknown(not the strict shape the public function declares), narrow withisValidString/isIndexableObjectfrom@datadog/browser-core, and pin the runtime contract with tests that bypass the type system (badValue as unknown as ExpectedType).NonNullable<RumVitalOperationStepEvent['vital']>['step_type']) instead of duplicating literals. Schema regenerations propagate automatically.displayErrorvsdisplayWarn— drop-vs-emit semantics.displayErrorfor "we're refusing to do what you asked";displayWarnfor "we're still emitting but flagging something suspicious".docs/DEVELOPMENT.md— new "Auto-Generated Files" table:CHANGELOG.md(generated byyarn release) and the existingsrc/rumEvent.types.tsrule into a single, discoverable reference. Reviewers on PR ⚗️ [RUM-15521] add RUM Operations API to the main process #102 flagged the manual changelog edit immediately — this should head off the same thing for future contributors.Test instructions
Visual review of the diff. No runtime impact.
Checklist