-
Notifications
You must be signed in to change notification settings - Fork 11.8k
docs: expand coding, testing, and PR guidelines in AGENTS.md #34183
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.
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,15 @@ This is the source code for the Angular CLI and related build tooling. This guid | |
| pnpm build --local | ||
| ``` | ||
|
|
||
| ## Coding Practices | ||
|
|
||
| - **Imports:** | ||
| - Always use the `node:` protocol for Node.js built-in imports (e.g., `node:fs`, `node:path`, `node:assert`). | ||
| - Prefer named imports (e.g., `import { mkdtemp } from 'node:fs'`) or default imports (`import fs from 'node:fs'`) instead of namespace imports (`import * as fs`). | ||
| - Use type-only imports (`import type { ... }`) when importing types to avoid runtime side-effects. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider: IMHO, it's usually fine to let import elision drop a symbol used only in type locations. In my experience, the only case you really need to be explicit is
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For AI agents specifically, being explicit helps avoid circular runtime dependency traps and accidental runtime imports when importing types from files that have top-level side effects. Single-file transpilers (like esbuild or Babel) also benefit from explicit |
||
| - **Classes:** | ||
| - Prefer ECMAScript private fields (`#field`) over TypeScript `private` keywords for encapsulated state. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question: I thought ECMAScript private fields were problematic in g3? Is there a particular motivation for preferring them?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We've never really had problems with ECMAScript private fields in g3. Beyond that,
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like I've seen Copybara CLs which error because they fail to compile ECMAScript private fields? From a quick search, I don't see any existing private symbols synced into g3: http://cs/search?q=%22%23%22%20lang:typescript&sq=&ss=piper%2FGoogle%2FPiper:google3%2Fthird_party%2Fjavascript%2Fangular_cli%2F As long as we can sync successfully, I don't have any major concerns. Just want to make sure that doesn't become an issue.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do use privates in the CLI repo code, but I think it's code that is not synced. I can remove it if you wish.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My motivation is 1. make sure syncing doesn't fail and 2. have consistent rules for the whole repo. Maybe just test if ECMAScript privates fail in g3 (we don't really go through Closure Compiler, so maybe it's fine and I'm just hallucinating?). If they work, then no objection from me. If they don't, then I'd suggest using TS |
||
|
|
||
| ## Testing | ||
|
|
||
| - **Temporary Directories (`TEST_TMPDIR`):** | ||
|
|
@@ -42,14 +51,15 @@ This is the source code for the Angular CLI and related build tooling. This guid | |
| }); | ||
| ``` | ||
| - **NEVER** use or fallback to `os.tmpdir()`. Bazel executes tests in hermetic sandboxes and sets `TEST_TMPDIR` to an isolated, sandboxed directory. Using `os.tmpdir()` can cause sandboxing failures, permission errors, or file leakage outside the Bazel sandbox. | ||
| - **Imports:** | ||
| - Always use the `node:` protocol for Node.js built-in imports (e.g., `node:fs`, `node:path`, `node:assert`). | ||
| - Prefer named imports (e.g., `import { mkdtemp } from 'node:fs'`) or default imports (`import fs from 'node:fs'`) instead of namespace imports (`import * as fs`). | ||
| - **Unit Tests:** | ||
| - Run all unit tests: `pnpm bazel test //packages/...` | ||
| - Run a specific test target: `pnpm bazel test //packages/angular/build:test` | ||
| - Query test targets: `pnpm bazel query "tests(//packages/...)"` | ||
| - Focus specific tests when debugging: use `fdescribe()` and `fit()`. NEVER commit focused tests to the repository. | ||
| - Run tests without sharding when isolating or debugging: use `--config=no-sharding` with a specific test target (e.g., `pnpm bazel test //packages/angular/build:test --config=no-sharding`). | ||
| This disables test sharding (`--test_sharding_strategy=disabled`) and flaky test retries (`--flaky_test_attempts=1`). | ||
| This is especially useful when isolating test runs or debugging with focused tests (`fit`/`fdescribe`) to avoid empty shard failures and unnecessary re-runs. | ||
| Do not use this flag when running broad test suites (such as `//packages/...`), as executing tests without sharding takes significantly longer. | ||
| - **End-to-End Tests:** | ||
| - Run subset of E2E tests: `pnpm bazel test //tests:e2e_node22 --config=e2e --test_filter="<filter>"` | ||
|
|
||
|
|
@@ -58,6 +68,7 @@ This is the source code for the Angular CLI and related build tooling. This guid | |
| - Use the `gh` CLI (GitHub CLI) for creating and managing pull requests. | ||
| - **Fixup Commits:** | ||
| - When addressing review feedback, **ALWAYS** use fixup commits (`git commit --fixup <commit>`) instead of amending existing commits. This preserves commit history during review and allows reviewers to easily see incremental changes. | ||
| - Only use fixup commits for changes that directly belong to the target commit. Unrelated changes must be made in a separate commit with their own commit message, not as a fixup commit. | ||
| - Fixup commits are automatically squashed when merging with `pnpm ng-dev pr merge` or rebasing with `pnpm ng-dev pr rebase <pr>`. | ||
| - Use `pnpm ng-dev pr` commands: | ||
| - `pnpm ng-dev pr rebase <pr>`: Rebase a PR branch on its target branch and squash fixup commits. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: What's the motivation for default imports over namespace imports? I feel like I've personally had less issues with the later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Named imports are preferred when specific symbols are needed. Between default (
import fs from 'node:fs') and namespace (import * as fs from 'node:fs'), namespace imports produce frozen module namespace exotic objects that can cause bundling and CJS/ESM interop quirks. Default and named imports align directly with Node's native ESM resolution and modern bundlers. (This guideline was also already inAGENTS.mdunder Testing and was moved here to apply generally).