|
|
@@ -1,6 +1,6 @@
|
|
|
# Coding Style
|
|
|
|
|
|
-General coding standards and best practices. These rules apply to all code in the GROWI monorepo.
|
|
|
+General coding standards for all code in the GROWI monorepo.
|
|
|
|
|
|
## Immutability (CRITICAL)
|
|
|
|
|
|
@@ -8,32 +8,20 @@ ALWAYS create new objects, NEVER mutate:
|
|
|
|
|
|
```typescript
|
|
|
// ❌ WRONG: Mutation
|
|
|
-function updateUser(user, name) {
|
|
|
- user.name = name // MUTATION!
|
|
|
- return user
|
|
|
-}
|
|
|
-
|
|
|
-// ✅ CORRECT: Immutability
|
|
|
-function updateUser(user, name) {
|
|
|
- return {
|
|
|
- ...user,
|
|
|
- name
|
|
|
- }
|
|
|
-}
|
|
|
-
|
|
|
-// ✅ CORRECT: Array immutable update
|
|
|
-const updatedPages = pages.map(p => p.id === id ? { ...p, title: newTitle } : p);
|
|
|
-
|
|
|
-// ❌ WRONG: Array mutation
|
|
|
+user.name = name;
|
|
|
pages[index].title = newTitle;
|
|
|
+
|
|
|
+// ✅ CORRECT: Immutable update
|
|
|
+return { ...user, name };
|
|
|
+const updatedPages = pages.map(p => (p.id === id ? { ...p, title: newTitle } : p));
|
|
|
```
|
|
|
|
|
|
## File Organization
|
|
|
|
|
|
MANY SMALL FILES > FEW LARGE FILES:
|
|
|
|
|
|
-- High cohesion, low coupling
|
|
|
-- 200-400 lines typical, 800 max
|
|
|
+- **High cohesion, low coupling** — every symbol in a file should share a single responsibility; if you struggle to name the file, it probably has too many
|
|
|
+- 200–400 lines typical, 800 max
|
|
|
- Functions < 50 lines
|
|
|
- Extract utilities from large components
|
|
|
- Organize by feature/domain, not by type
|
|
|
@@ -42,20 +30,13 @@ MANY SMALL FILES > FEW LARGE FILES:
|
|
|
|
|
|
### Pure Function Extraction
|
|
|
|
|
|
-When a framework-specific wrapper (React hook, Express middleware, CodeMirror extension handler, etc.) contains non-trivial logic, extract the core logic as a **pure function** and reduce the wrapper to a thin adapter. This enables direct reuse across different contexts and makes unit testing straightforward.
|
|
|
+When a framework-specific wrapper (React hook, Express middleware, CodeMirror extension handler, etc.) contains non-trivial logic, extract the core logic as a **pure function** and reduce the wrapper to a thin adapter. This enables reuse across contexts and makes unit testing straightforward.
|
|
|
|
|
|
```typescript
|
|
|
-// ❌ WRONG: Business logic locked inside a framework-specific wrapper
|
|
|
-export const useToggleSymbol = (view?: EditorView) => {
|
|
|
- return useCallback((prefix, suffix) => {
|
|
|
- // 30 lines of symbol-toggling logic here...
|
|
|
- }, [view]);
|
|
|
-};
|
|
|
-
|
|
|
-// ✅ CORRECT: Pure function + thin wrappers for each context
|
|
|
+// ✅ Pure function — testable, reusable from hooks, keymaps, shortcuts, etc.
|
|
|
// services-internal/markdown-utils/toggle-markdown-symbol.ts
|
|
|
export const toggleMarkdownSymbol = (view: EditorView, prefix: string, suffix: string): void => {
|
|
|
- // Pure logic — testable, reusable from hooks, keymaps, shortcuts, etc.
|
|
|
+ // Pure logic
|
|
|
};
|
|
|
|
|
|
// React hook wrapper
|
|
|
@@ -83,7 +64,7 @@ Replace conditional branching on mode/variant names with **declared metadata** t
|
|
|
```typescript
|
|
|
// ❌ WRONG: Consumer knows mode-specific behavior
|
|
|
if (keymapModeName === 'emacs') {
|
|
|
- return sharedKeyBindings; // exclude formatting
|
|
|
+ return sharedKeyBindings;
|
|
|
}
|
|
|
return [formattingBindings, ...sharedKeyBindings];
|
|
|
|
|
|
@@ -96,7 +77,7 @@ const activeBindings = allGroups
|
|
|
|
|
|
### Factory Pattern with Encapsulated Metadata
|
|
|
|
|
|
-When a module produces a value that requires configuration from the consumer (precedence, feature flags, etc.), **bundle the metadata alongside the value** in a structured return type. This keeps decision-making inside the module that has the knowledge.
|
|
|
+When a module produces a value that needs consumer-side configuration (precedence, feature flags, etc.), **bundle the metadata alongside the value** in a structured return type. This keeps decision-making inside the module that has the knowledge.
|
|
|
|
|
|
```typescript
|
|
|
// ❌ WRONG: Consumer decides precedence based on mode name
|
|
|
@@ -115,7 +96,7 @@ codeMirrorEditor.appendExtensions(result.precedence(result.extension));
|
|
|
|
|
|
### Responsibility-Based Submodule Decomposition
|
|
|
|
|
|
-When a single module grows beyond ~200 lines or accumulates multiple distinct responsibilities, split into submodules **by responsibility domain** (not by arbitrary size). Each submodule should be independently understandable.
|
|
|
+When a module grows beyond ~200 lines or accumulates multiple distinct responsibilities, split into submodules **by responsibility domain** (not by arbitrary size). Each submodule should be independently understandable.
|
|
|
|
|
|
```
|
|
|
// ❌ WRONG: One large file with mixed concerns
|
|
|
@@ -129,82 +110,77 @@ keymaps/emacs/
|
|
|
└── navigation.ts ← Movement and editing commands
|
|
|
```
|
|
|
|
|
|
-## Naming Conventions
|
|
|
+## Module Public Surface: Barrel Files & Directory Boundaries
|
|
|
|
|
|
-### Variables and Functions
|
|
|
+Treat a directory as a **module with a single public entry point** (`index.ts`). The barrel declares the public API; everything else is an implementation detail.
|
|
|
|
|
|
-- **camelCase** for variables and functions
|
|
|
-- **PascalCase** for classes, interfaces, types, React components
|
|
|
-- **UPPER_SNAKE_CASE** for constants
|
|
|
+### Rules
|
|
|
|
|
|
-```typescript
|
|
|
-const pageId = '123';
|
|
|
-const MAX_PAGE_SIZE = 1000;
|
|
|
+1. **One barrel per module directory.** `index.ts` is the sole export surface. Siblings/parents import only from the barrel, never reach into internal files.
|
|
|
+2. **Re-export only what callers need.** If a symbol has no external caller, do not add it to the barrel. Expanding the surface is a deliberate decision, not a default.
|
|
|
+3. **Nest subdirectories for cohesive internals.** When a module has a cluster of related files (DOM rendering, parsers, adapters), move them into a subdirectory with its own barrel. The parent barrel imports from that subdirectory without leaking its internals.
|
|
|
+4. **Prefer `import { X } from './dom'` over `import { X } from './dom/widget'`.** Reaching through a barrel keeps the coupling at the module level, not the file level.
|
|
|
|
|
|
-function getPageById(id: string) { }
|
|
|
-class PageService { }
|
|
|
-interface PageData { }
|
|
|
-type PageStatus = 'draft' | 'published';
|
|
|
+### Example (from `y-rich-cursors/` refactor)
|
|
|
+
|
|
|
+```
|
|
|
+y-rich-cursors/
|
|
|
+├── index.ts ← Public API: only yRichCursors() + YRichCursorsOptions
|
|
|
+├── plugin.ts ← Internal orchestrator
|
|
|
+├── activity-tracker.ts ← Internal
|
|
|
+├── local-cursor.ts ← Internal
|
|
|
+├── viewport-classification.ts ← Internal
|
|
|
+└── dom/
|
|
|
+ ├── index.ts ← Sub-barrel: exposes widget/theme/indicator to siblings only
|
|
|
+ ├── widget.ts
|
|
|
+ ├── theme.ts
|
|
|
+ └── off-screen-indicator.ts
|
|
|
```
|
|
|
|
|
|
-### Files and Directories
|
|
|
+Before the refactor, `index.ts` re-exported `RichCaretWidget`, `createOffScreenIndicator`, `ScrollCallbackRef`, etc. — internal details leaked as public API. After: the top-level barrel exposes the single `yRichCursors` entry point, and DOM concerns live behind `dom/index.ts` so that sibling modules (`plugin.ts`) can consume them without making them part of the module's external contract.
|
|
|
+
|
|
|
+**When adding a new file, ask**: is this intended for external callers? If no, it does not belong in the top-level barrel. If it is one of several related internals, consider a subdirectory.
|
|
|
|
|
|
-- **PascalCase** for React components: `Button.tsx`, `PageTree.tsx`
|
|
|
-- **kebab-case** for utilities: `page-utils.ts`
|
|
|
-- **lowercase** for directories: `features/page-tree/`, `utils/`
|
|
|
+## Naming Conventions
|
|
|
+
|
|
|
+- **camelCase** — variables, functions
|
|
|
+- **PascalCase** — classes, interfaces, types, React components
|
|
|
+- **UPPER_SNAKE_CASE** — constants
|
|
|
+- **PascalCase** file — React components (`Button.tsx`)
|
|
|
+- **kebab-case** file — utilities (`page-utils.ts`)
|
|
|
+- **lowercase** directory — `features/page-tree/`, `utils/`
|
|
|
|
|
|
## Export Style
|
|
|
|
|
|
**Prefer named exports** over default exports:
|
|
|
|
|
|
```typescript
|
|
|
-// ✅ Good: Named exports
|
|
|
+// ✅ Good
|
|
|
export const MyComponent = () => { };
|
|
|
-export function myFunction() { }
|
|
|
-export class MyClass { }
|
|
|
|
|
|
-// ❌ Avoid: Default exports
|
|
|
+// ❌ Avoid (exception: Next.js pages)
|
|
|
export default MyComponent;
|
|
|
```
|
|
|
|
|
|
-**Why?**
|
|
|
-- Better refactoring (IDEs can reliably rename across files)
|
|
|
-- Better tree shaking
|
|
|
-- Explicit imports improve readability
|
|
|
-- No ambiguity (import name matches export name)
|
|
|
-
|
|
|
-**Exception**: Next.js pages require default exports.
|
|
|
+Named exports give reliable IDE rename, better tree shaking, and unambiguous import names.
|
|
|
|
|
|
## Type Safety
|
|
|
|
|
|
-**Always provide explicit types** for function parameters and return values:
|
|
|
+Always provide explicit types for function parameters and return values. Use `import type` for type-only imports.
|
|
|
|
|
|
```typescript
|
|
|
-// ✅ Good: Explicit types
|
|
|
-function createPage(path: string, body: string): Promise<Page> {
|
|
|
- // ...
|
|
|
-}
|
|
|
-
|
|
|
-// ❌ Avoid: Implicit any
|
|
|
-function createPage(path, body) {
|
|
|
- // ...
|
|
|
-}
|
|
|
-```
|
|
|
-
|
|
|
-Use `import type` for type-only imports:
|
|
|
+function createPage(path: string, body: string): Promise<Page> { /* ... */ }
|
|
|
|
|
|
-```typescript
|
|
|
import type { PageData } from '~/interfaces/page';
|
|
|
```
|
|
|
|
|
|
## Error Handling
|
|
|
|
|
|
-ALWAYS handle errors comprehensively:
|
|
|
+Handle errors comprehensively — log with context, rethrow with a user-friendly message:
|
|
|
|
|
|
```typescript
|
|
|
try {
|
|
|
- const result = await riskyOperation();
|
|
|
- return result;
|
|
|
+ return await riskyOperation();
|
|
|
} catch (error) {
|
|
|
logger.error('Operation failed:', { error, context });
|
|
|
throw new Error('Detailed user-friendly message');
|
|
|
@@ -213,68 +189,23 @@ try {
|
|
|
|
|
|
## Async/Await
|
|
|
|
|
|
-Prefer async/await over Promise chains:
|
|
|
-
|
|
|
-```typescript
|
|
|
-// ✅ Good: async/await
|
|
|
-async function loadPages() {
|
|
|
- const pages = await fetchPages();
|
|
|
- const enriched = await enrichPageData(pages);
|
|
|
- return enriched;
|
|
|
-}
|
|
|
-
|
|
|
-// ❌ Avoid: Promise chains
|
|
|
-function loadPages() {
|
|
|
- return fetchPages()
|
|
|
- .then(pages => enrichPageData(pages))
|
|
|
- .then(enriched => enriched);
|
|
|
-}
|
|
|
-```
|
|
|
+Prefer `async/await` over `.then()` chains.
|
|
|
|
|
|
## Comments
|
|
|
|
|
|
-**Write comments in English** (even for Japanese developers):
|
|
|
-
|
|
|
-```typescript
|
|
|
-// ✅ Good: English comment
|
|
|
-// Calculate the total number of pages in the workspace
|
|
|
-
|
|
|
-// ❌ Avoid: Japanese comment
|
|
|
-// ワークスペース内のページ総数を計算
|
|
|
-```
|
|
|
-
|
|
|
-**When to comment**:
|
|
|
-- Complex algorithms or business logic
|
|
|
-- Non-obvious workarounds
|
|
|
-- Public APIs and interfaces
|
|
|
-
|
|
|
-**When NOT to comment**:
|
|
|
-- Self-explanatory code (good naming is better)
|
|
|
-- Restating what the code does
|
|
|
+**Write comments in English.** Only comment when the WHY is non-obvious (hidden constraints, invariants, workarounds). Do not restate what the code does — let naming do that work.
|
|
|
|
|
|
## Test File Placement
|
|
|
|
|
|
-**Co-locate tests with source files** in the same directory:
|
|
|
-
|
|
|
-```
|
|
|
-src/utils/
|
|
|
-├── helper.ts
|
|
|
-└── helper.spec.ts # Test next to source
|
|
|
-
|
|
|
-src/components/Button/
|
|
|
-├── Button.tsx
|
|
|
-└── Button.spec.tsx # Test next to component
|
|
|
-```
|
|
|
-
|
|
|
-### Test File Naming
|
|
|
+Co-locate tests with source files.
|
|
|
|
|
|
-- Unit tests: `*.spec.{ts,js}`
|
|
|
-- Integration tests: `*.integ.ts`
|
|
|
-- Component tests: `*.spec.{tsx,jsx}`
|
|
|
+- Unit: `*.spec.{ts,js}`
|
|
|
+- Integration: `*.integ.ts`
|
|
|
+- Component: `*.spec.{tsx,jsx}`
|
|
|
|
|
|
## Git Commit Messages
|
|
|
|
|
|
-Follow conventional commit format:
|
|
|
+Conventional commits:
|
|
|
|
|
|
```
|
|
|
<type>(<scope>): <subject>
|
|
|
@@ -282,33 +213,20 @@ Follow conventional commit format:
|
|
|
<body>
|
|
|
```
|
|
|
|
|
|
-**Types**: `feat`, `fix`, `refactor`, `test`, `docs`, `chore`
|
|
|
-
|
|
|
-**Example**:
|
|
|
-```
|
|
|
-feat(page-tree): add virtualization for large trees
|
|
|
-
|
|
|
-Implemented react-window for virtualizing page tree
|
|
|
-to improve performance with 10k+ pages.
|
|
|
-```
|
|
|
+Types: `feat`, `fix`, `refactor`, `test`, `docs`, `chore`.
|
|
|
|
|
|
## Cross-Platform Compatibility
|
|
|
|
|
|
GROWI must work on Windows, macOS, and Linux. Never use platform-specific shell commands in npm scripts.
|
|
|
|
|
|
```json
|
|
|
-// ❌ WRONG: Unix-only commands in npm scripts
|
|
|
-"clean": "rm -rf dist",
|
|
|
-"copy": "cp src/foo.ts dist/foo.ts",
|
|
|
-"move": "mv src dist"
|
|
|
-
|
|
|
-// ✅ CORRECT: Cross-platform tools
|
|
|
-"clean": "rimraf dist",
|
|
|
-"copy": "node -e \"require('fs').cpSync('src/foo.ts','dist/foo.ts')\"",
|
|
|
-"move": "node -e \"require('fs').renameSync('src','dist')\""
|
|
|
+// ❌ WRONG: Unix-only
|
|
|
+"clean": "rm -rf dist"
|
|
|
+
|
|
|
+// ✅ CORRECT: Cross-platform
|
|
|
+"clean": "rimraf dist"
|
|
|
```
|
|
|
|
|
|
-**Rules**:
|
|
|
- Use `rimraf` instead of `rm -rf`
|
|
|
- Use Node.js one-liners or cross-platform tools (`cpy-cli`, `cpx2`) instead of `cp`, `mv`, `echo`, `ls`
|
|
|
- Never assume a POSIX shell in npm scripts
|
|
|
@@ -319,10 +237,10 @@ Before marking work complete:
|
|
|
|
|
|
- [ ] Code is readable and well-named
|
|
|
- [ ] Functions are small (<50 lines)
|
|
|
-- [ ] Files are focused (<800 lines)
|
|
|
+- [ ] Files are focused (<800 lines) and **highly cohesive** (one responsibility per file; filename describes all symbols)
|
|
|
- [ ] No deep nesting (>4 levels)
|
|
|
- [ ] Proper error handling
|
|
|
-- [ ] No console.log statements (use logger)
|
|
|
+- [ ] No `console.log` (use logger)
|
|
|
- [ ] No mutation (immutable patterns used)
|
|
|
- [ ] Named exports (except Next.js pages)
|
|
|
- [ ] English comments
|
|
|
@@ -330,3 +248,5 @@ Before marking work complete:
|
|
|
- [ ] Non-trivial logic extracted as pure functions from framework wrappers
|
|
|
- [ ] No hard-coded mode/variant checks in consumers (use declared metadata)
|
|
|
- [ ] Modules with multiple responsibilities split by domain
|
|
|
+- [ ] **Module public surface is minimal** — `index.ts` re-exports only what external callers need; internals stay unexported
|
|
|
+- [ ] **Cohesive internals are grouped in subdirectories** with their own barrel, not flattened into the parent
|