refactor(components): use generic type params more consistently#5
refactor(components): use generic type params more consistently#5
Conversation
nberlette
commented
Mar 10, 2026
- refactor(components): use generic type params more consistently
- fix: update broken import path in component.ts
- refactor(events): use readonly array for EventEmitter type constraint
- fix(tui): updating broken import in tui.ts
- feat(internal): support thisArg in SortedArray.sort method
- refactor(utils): use more robust global object resolution
- fix(canvas): add all missing override modifiers
- fix(input): remove stray text from mouse.ts
- fix: broken import paths
- chore: fmt
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1361a413bb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return (0, eval)("this") || self || window || globalThis || global; | ||
| } catch (_) { | ||
| // deno-lint-ignore no-node-globals | ||
| return self || window || globalThis || global; |
There was a problem hiding this comment.
Guard global fallbacks with typeof checks
If eval is unavailable (for example in Node with --disallow-code-generation-from-strings or other CSP-restricted runtimes), execution enters the catch branch and directly references self/window. In environments where those identifiers are undeclared, return self || window || globalThis || global; throws ReferenceError before reaching globalThis, so importing this module fails at startup. Using typeof checks for each global avoids turning the fallback path into a hard crash.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR refactors internal input handling and component typing, while also adjusting a couple of low-level utilities (sorted array sorting and runtime global detection).
Changes:
- Replace
input_readerimports/exports with the newinputmodule paths across the public surface and core classes. - Tighten component
drawnObjectstyping by passing the shape viaComponent/Boxgenerics instead ofdeclare drawnObjects. - Extend
SortedArray.sorttyping to support a comparatorthiscontext and add minor cleanup/override annotations.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/sorted_array.ts | Extends sort signature to support thisArg binding for comparators. |
| src/utils/runtime.ts | Reworks global object detection logic used by runtime detection helpers. |
| src/tui.ts | Updates InputEventRecord import to come from src/input/mod.ts. |
| src/layout/mod.ts | Reorders exports (moves grid_layout export up). |
| src/input/mouse.ts | Removes stray unreachable text (SSS;) in mouse decode. |
| src/event_emitter.ts | Adjusts EmitterEvent args type constraint to readonly unknown[]. |
| src/components/textbox.ts | Updates KeyPressEvent import to src/input/types.ts. |
| src/components/text.ts | Switches to Component<{ text: TextObject }> generic for drawnObjects typing. |
| src/components/table.ts | Switches to Component<...> generic for drawnObjects typing. |
| src/components/slider.ts | Switches to Box<...> generic for drawnObjects typing. |
| src/components/label.ts | Switches to Component<{ texts: TextObject[] }> generic for drawnObjects typing. |
| src/components/input.ts | Switches to Box<...> generic for drawnObjects typing. |
| src/components/frame.ts | Switches to Component<...> generic for drawnObjects typing. |
| src/component.ts | Updates InputEventRecord import to come from src/input/mod.ts. |
| src/canvas/text.ts | Adds override keywords to methods overriding base class behavior. |
| mod.ts | Updates public export from ./src/input_reader/mod.ts to ./src/input/mod.ts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| // deno-lint-ignore no-node-globals | ||
| return (0, eval)("this") || self || window || globalThis || global; | ||
| } catch (_) { | ||
| // deno-lint-ignore no-node-globals | ||
| return self || window || globalThis || global; | ||
| } |
There was a problem hiding this comment.
_global initialization can throw ReferenceError in environments where self, window, globalThis, or global are not defined. Because those identifiers are referenced directly in both the try and catch branches, the catch can also throw, defeating the fallback and potentially breaking module evaluation. Use typeof guards (e.g. typeof self !== "undefined") and/or prefer globalThis when available without referencing undeclared globals; also consider avoiding eval for CSP-restricted environments.
| try { | |
| // deno-lint-ignore no-node-globals | |
| return (0, eval)("this") || self || window || globalThis || global; | |
| } catch (_) { | |
| // deno-lint-ignore no-node-globals | |
| return self || window || globalThis || global; | |
| } | |
| if (typeof globalThis !== "undefined") { | |
| return globalThis; | |
| } | |
| if (typeof self !== "undefined") { | |
| return self; | |
| } | |
| if (typeof window !== "undefined") { | |
| return window; | |
| } | |
| // deno-lint-ignore no-node-globals | |
| if (typeof global !== "undefined") { | |
| return global; | |
| } | |
| return {}; |
| thisArg?: This, | ||
| ): this { | ||
| compareFn ??= this.compareFn; | ||
| if (thisArg) compareFn = compareFn?.bind(thisArg); |
There was a problem hiding this comment.
if (thisArg) will skip binding when thisArg is a valid falsy value (e.g. 0, "", false), so callers providing such a thisArg will silently not get the intended this in compareFn. Check for thisArg !== undefined (or arguments.length >= 2) instead of truthiness so binding behavior matches the provided argument.
| if (thisArg) compareFn = compareFn?.bind(thisArg); | |
| if (thisArg !== undefined) compareFn = compareFn?.bind(thisArg); |
- [x] add a dedicated ./signals/errors submodule - [x] move error subclasses out of signal.ts and computed.ts - [x] improve some other minor things
refactor: improve signals APIs, add `effect` and `computed` helpers