Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`).

Copy link
Copy Markdown
Collaborator

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.

Copy link
Copy Markdown
Collaborator Author

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 in AGENTS.md under Testing and was moved here to apply generally).

- Use type-only imports (`import type { ... }`) when importing types to avoid runtime side-effects.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 export type {...} from '...'; (which is necessary to be compatible with isolatedModules) or if importing the real implementation introduces an error state (ex. DevTools should always import type {...} from '@angular/core';, because the real implementation is a different version than the application being inspected).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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 import type to guarantee no side-effect imports are preserved.

- **Classes:**
- Prefer ECMAScript private fields (`#field`) over TypeScript `private` keywords for encapsulated state.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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?

http://go/tsjs-style#private-fields

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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, #field provides true runtime encapsulation and prevents agents or callers from accessing private internals via type assertions or index signatures.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The 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.

https://github.com/search?q=repo%3Aangular%2Fangular-cli+%23+typescript+language%3ATypeScript&type=code&l=TypeScript&p=1

I can remove it if you wish.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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 private instead just for consistency the Angular CLI repo.


## Testing

- **Temporary Directories (`TEST_TMPDIR`):**
Expand All @@ -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>"`

Expand All @@ -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.
Expand Down
Loading