Platform Adapter #39
Platform Adapter #39
Conversation
Restructure edge worker to support runtime-specific environments with a new platform adapter pattern. Key changes include: - Introduce PlatformAdapter interface for cross-runtime compatibility - Create DenoAdapter with environment-specific logging and worker management - Replace static logger with dynamic logger creation - Remove environment-specific code from core worker classes - Add platform detection and adapter initialization mechanism - Enhance worker configuration and environment handling
Remove custom logger implementation and update components to use injected logger - Delete local Logger.ts module - Replace getLogger() calls with explicit logger injection - Update constructors in multiple components to accept logger parameter - Migrate logger imports to platform/types.js - Simplify logger initialization across edge worker components
…ialization Add logger parameter to WorkerState constructor and Heartbeat initialization in both lifecycle classes, ensuring consistent logging across worker components
…pe safety and logging Add Deno type definitions, improve environment detection, and implement more robust logging and error handling for edge worker platform adapter
Simplified and strengthened type checking for Deno and EdgeRuntime environments by: - Consolidating environment validation logic in DenoAdapter - Updating global type declarations to be more strict - Replacing multiple console warnings with a single error throw
Refactored class initialization by removing default WorkerState instantiation and adjusting Heartbeat constructor call to use explicit parameter placement
Update TypeScript configuration across multiple packages to include node and deno type definitions, and remove global node types from base configuration
…zation Remove unnecessary type definitions and simplify platform adapter initialization Adjust type for EdgeRuntime waitUntil method to use unknown type Add comment about environment support limitations
…neration Strengthen type definitions for worker and request handling in Deno environment Remove fallback UUID generation for execution ID
Replaced "@types/deno" with "@teidesu/deno-types" in package.json Updated tsconfig.lib.json to use new type definitions
…integration Refactor EdgeWorker to improve platform adapter initialization and worker creation process. Key changes include: - Modify adapter initialization to accept a worker factory function - Remove static worker and separate initialization logic - Introduce CreateWorkerFn and CreateLoggerFn type definitions - Simplify start and stop methods in EdgeWorker - Update DenoAdapter to work with new initialization pattern Breaking changes: The EdgeWorker initialization process has been significantly modified
Add explicit validation for SB_EXECUTION_ID environment variable and use variable shorthand
WalkthroughThis pull request introduces numerous refactorings and additions. TypeScript configuration files have been updated with Node.js type definitions and adjusted options. Logging mechanisms throughout the edge-worker package have been refactored to remove static instantiations in favor of dependency injection, with constructors updated accordingly. New platform-specific functionality has been added via the DenoAdapter and createAdapter, along with a logging factory and new type declarations. Package management, ESLint, build targets, CI workflow configurations, and tests have also been revised, and deprecated functions removed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant EW as EdgeWorker
participant PA as PlatformAdapter (DenoAdapter)
participant LF as Logger Factory
Client->>EW: start()
EW->>PA: await createAdapter()
PA-->>EW: return adapter instance
EW->>LF: create logger instance
EW->>EW: initialize worker with adapter & logger
Client->>EW: stop()
EW->>PA: stopWorker()
PA-->>EW: worker stopped
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
View your CI Pipeline Execution ↗ for commit c65f6ee.
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
pkgs/edge-worker/src/platform/types.ts (1)
22-30: Consider adding data typing for environment variables.The PlatformEnvironment interface provides a good starting point for platform-specific environment variables.
Consider adding more specific types for the environment variables rather than using string for all of them:
export interface PlatformEnvironment { executionId: string; - logLevel: string; + logLevel: 'debug' | 'info' | 'warn' | 'error'; connectionString: string; // Add other environment variables as needed }pkgs/edge-worker/src/core/ExecutionController.ts (1)
48-50: Fix typo in template string.There's an extra curly brace in the template string for logging.
this.logger.debug( - `Awaiting completion of all tasks... (active/all: ${active}}/${all})` + `Awaiting completion of all tasks... (active/all: ${active}/${all})` );pkgs/edge-worker/src/EdgeWorker.ts (3)
38-38: Consider clarifying the adapter property naming.
Whileadapteris accurate, a more descriptive name such asplatformAdaptermight improve clarity.- private static adapter: PlatformAdapter | null = null; + private static platformAdapter: PlatformAdapter | null = null;
81-81: Static analysis note about usingthisin static methods.
Biome flagged multiple lines indicating “Using this in a static context can be confusing.” Although acceptable in TypeScript, you could replacethiswith the class name (EdgeWorker) for clarity.- this.ensureFirstCall(); + EdgeWorker.ensureFirstCall(); - this.adapter = await createAdapter(); + EdgeWorker.adapter = await createAdapter(); - const env = this.adapter.getEnv(); + const env = EdgeWorker.adapter.getEnv(); - await this.adapter.initialize(createWorkerFn); + await EdgeWorker.adapter.initialize(createWorkerFn); - return this.adapter; + return EdgeWorker.adapter; - if (this.adapter) { + if (EdgeWorker.adapter) {Also applies to: 84-84, 87-87, 109-109, 111-111, 118-119
🧰 Tools
🪛 Biome (1.9.4)
[error] 81-81: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
100-100: Check for fallback logic if environment variable is unset.
connectionString: config.connectionString || env.connectionStringis straightforward, but if both are absent or invalid, the code might fail later in unexpected ways. Consider adding a more direct error or fallback.pkgs/edge-worker/src/platform/DenoAdapter.ts (2)
28-53: Potential improvement for request handling.
The code returns a 200 response (“ok”) for every incoming HTTP request. Consider logging or handling unexpected conditions beyond the first request, e.g., ifthis.workeris already initialized.
115-144: Edge function spawning relies on external environment variables.
The code throws if the function name is empty and checks for a non-OK response. This is good. Consider exposing more detailed diagnostics iffetchfails to connect.pkgs/edge-worker/src/platform/index.ts (1)
1-4: Platform adapter exports look good, but consider documenting the importThe exports properly establish the platform adapter interface, allowing for platform-agnostic code. The explicit exports of
createAdapterandDenoAdapterprovide the necessary components for the cross-platform functionality.Consider adding a brief comment explaining why
deno-types.jsis imported without being exported:export * from './types.js'; export { createAdapter } from './createAdapter.js'; export { DenoAdapter } from './DenoAdapter.js'; +// Import deno-types for its side effects (type registration) import './deno-types.js';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (29)
pkgs/core/tsconfig.lib.json(1 hunks)pkgs/dsl/tsconfig.lib.json(1 hunks)pkgs/edge-worker/package.json(1 hunks)pkgs/edge-worker/src/EdgeWorker.ts(5 hunks)pkgs/edge-worker/src/core/BatchProcessor.ts(1 hunks)pkgs/edge-worker/src/core/ExecutionController.ts(1 hunks)pkgs/edge-worker/src/core/Heartbeat.ts(1 hunks)pkgs/edge-worker/src/core/Logger.ts(0 hunks)pkgs/edge-worker/src/core/Worker.ts(2 hunks)pkgs/edge-worker/src/core/WorkerLifecycle.ts(2 hunks)pkgs/edge-worker/src/core/WorkerState.ts(2 hunks)pkgs/edge-worker/src/flow/FlowWorkerLifecycle.ts(2 hunks)pkgs/edge-worker/src/flow/StepTaskExecutor.ts(2 hunks)pkgs/edge-worker/src/flow/StepTaskPoller.ts(2 hunks)pkgs/edge-worker/src/flow/createFlowWorker.ts(5 hunks)pkgs/edge-worker/src/index.ts(1 hunks)pkgs/edge-worker/src/platform/DenoAdapter.ts(1 hunks)pkgs/edge-worker/src/platform/createAdapter.ts(1 hunks)pkgs/edge-worker/src/platform/deno-types.ts(1 hunks)pkgs/edge-worker/src/platform/index.ts(1 hunks)pkgs/edge-worker/src/platform/types.ts(1 hunks)pkgs/edge-worker/src/queue/MessageExecutor.ts(3 hunks)pkgs/edge-worker/src/queue/Queue.ts(5 hunks)pkgs/edge-worker/src/queue/ReadWithPollPoller.ts(2 hunks)pkgs/edge-worker/src/queue/createQueueWorker.ts(4 hunks)pkgs/edge-worker/src/spawnNewEdgeFunction.ts(0 hunks)pkgs/edge-worker/tsconfig.lib.json(1 hunks)pkgs/example-flows/tsconfig.lib.json(1 hunks)tsconfig.base.json(0 hunks)
💤 Files with no reviewable changes (3)
- tsconfig.base.json
- pkgs/edge-worker/src/spawnNewEdgeFunction.ts
- pkgs/edge-worker/src/core/Logger.ts
🧰 Additional context used
🧬 Code Graph Analysis (15)
pkgs/edge-worker/src/platform/createAdapter.ts (3)
pkgs/edge-worker/src/platform/index.ts (2)
createAdapter(2-2)DenoAdapter(3-3)pkgs/edge-worker/src/platform/types.ts (1)
PlatformAdapter(35-51)pkgs/edge-worker/src/platform/DenoAdapter.ts (1)
DenoAdapter(12-184)
pkgs/edge-worker/src/core/ExecutionController.ts (2)
pkgs/edge-worker/src/core/types.ts (2)
IMessage(12-14)IExecutor(7-10)pkgs/edge-worker/src/platform/types.ts (1)
Logger(5-10)
pkgs/edge-worker/src/queue/MessageExecutor.ts (1)
pkgs/edge-worker/src/platform/types.ts (1)
Logger(5-10)
pkgs/edge-worker/src/core/Worker.ts (2)
pkgs/edge-worker/src/core/types.ts (1)
ILifecycle(16-28)pkgs/edge-worker/src/platform/types.ts (1)
Logger(5-10)
pkgs/edge-worker/src/queue/createQueueWorker.ts (6)
pkgs/edge-worker/src/platform/types.ts (1)
Logger(5-10)pkgs/edge-worker/src/queue/Queue.ts (1)
Queue(6-108)pkgs/edge-worker/src/core/WorkerLifecycle.ts (1)
WorkerLifecycle(12-95)pkgs/edge-worker/src/queue/ReadWithPollPoller.ts (1)
ReadWithPollPoller(13-46)pkgs/edge-worker/src/core/ExecutionController.ts (1)
ExecutionController(9-53)pkgs/edge-worker/src/core/BatchProcessor.ts (1)
BatchProcessor(5-40)
pkgs/edge-worker/src/flow/StepTaskExecutor.ts (2)
pkgs/edge-worker/src/platform/types.ts (1)
Logger(5-10)pkgs/core/src/types.ts (2)
StepTaskRecord(24-32)IPgflowClient(43-73)
pkgs/edge-worker/src/core/BatchProcessor.ts (3)
pkgs/edge-worker/src/core/types.ts (2)
IMessage(12-14)IPoller(3-5)pkgs/edge-worker/src/platform/types.ts (1)
Logger(5-10)pkgs/edge-worker/src/core/ExecutionController.ts (1)
ExecutionController(9-53)
pkgs/edge-worker/src/platform/types.ts (1)
pkgs/edge-worker/src/core/Worker.ts (1)
Worker(5-100)
pkgs/edge-worker/src/queue/ReadWithPollPoller.ts (3)
pkgs/edge-worker/src/platform/types.ts (1)
Logger(5-10)pkgs/edge-worker/src/queue/Queue.ts (1)
Queue(6-108)pkgs/edge-worker/src/queue/types.ts (1)
PgmqMessageRecord(8-14)
pkgs/edge-worker/src/flow/FlowWorkerLifecycle.ts (5)
pkgs/edge-worker/src/core/types.ts (2)
ILifecycle(16-28)WorkerRow(35-42)pkgs/edge-worker/src/core/WorkerState.ts (1)
WorkerState(38-89)pkgs/edge-worker/src/core/Heartbeat.ts (1)
Heartbeat(5-26)pkgs/edge-worker/src/platform/types.ts (1)
Logger(5-10)pkgs/edge-worker/src/core/Queries.ts (1)
Queries(4-44)
pkgs/edge-worker/src/core/WorkerLifecycle.ts (3)
pkgs/edge-worker/src/core/WorkerState.ts (1)
WorkerState(38-89)pkgs/edge-worker/src/core/Heartbeat.ts (1)
Heartbeat(5-26)pkgs/edge-worker/src/platform/types.ts (1)
Logger(5-10)
pkgs/edge-worker/src/core/WorkerState.ts (1)
pkgs/edge-worker/src/platform/types.ts (1)
Logger(5-10)
pkgs/edge-worker/src/flow/StepTaskPoller.ts (2)
pkgs/edge-worker/src/platform/types.ts (1)
Logger(5-10)pkgs/core/src/types.ts (1)
IPgflowClient(43-73)
pkgs/edge-worker/src/platform/DenoAdapter.ts (1)
pkgs/edge-worker/src/platform/types.ts (4)
PlatformAdapter(35-51)PlatformEnvironment(25-30)CreateWorkerFn(20-20)Logger(5-10)
pkgs/edge-worker/src/flow/createFlowWorker.ts (4)
pkgs/edge-worker/src/platform/types.ts (1)
Logger(5-10)pkgs/edge-worker/src/core/Worker.ts (1)
Worker(5-100)pkgs/edge-worker/src/flow/FlowWorkerLifecycle.ts (1)
FlowWorkerLifecycle(11-88)pkgs/edge-worker/src/flow/StepTaskExecutor.ts (1)
StepTaskExecutor(17-90)
🪛 Biome (1.9.4)
pkgs/edge-worker/src/EdgeWorker.ts
[error] 81-81: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 84-84: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 87-87: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 109-109: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 111-111: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 118-118: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
[error] 119-119: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.
(lint/complexity/noThisInStatic)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (63)
pkgs/edge-worker/package.json (1)
26-28: Appropriate addition of Deno type definitions.Adding Deno type definitions as a development dependency is an excellent approach to enable cross-platform development. This will provide proper TypeScript type checking for Deno APIs while developing in a Node.js environment.
pkgs/core/tsconfig.lib.json (1)
6-7: Properly configured Node.js type definitions.Explicitly including Node.js types in the TypeScript configuration ensures proper type checking when using Node.js APIs. This aligns well with the objective of enabling development on Node.js.
pkgs/edge-worker/src/platform/createAdapter.ts (3)
1-4: Well-structured imports for the adapter factory.The imports are organized correctly, bringing in the necessary interface, implementation, and type definitions.
5-17: Good factory pattern implementation for platform detection.The
createAdapterfunction implements a clean factory pattern that can be extended to support multiple platforms. Currently, it only supports Deno, which aligns with the PR objective.Consider implementing a NodeAdapter soon to avoid throwing errors when running in Node.js environments, as indicated by your TODO comments. This would complete the cross-platform functionality described in the PR objectives.
19-21: Simple and effective environment detection.The
isDenoEnvironmenthelper function uses a straightforward and reliable method to detect the Deno runtime by checking for the globalDenoobject.pkgs/edge-worker/tsconfig.lib.json (1)
6-7: Excellent dual-platform type configuration.Including both Node.js and Deno type definitions is perfect for implementing the cross-platform adapter pattern. This configuration enables developers to work in a Node.js environment while ensuring type safety when using Deno-specific APIs.
pkgs/edge-worker/src/index.ts (1)
9-11: Platform adapter exports added to main package indexThese exports make the platform adapter functionality available to consumers of the edge-worker package, which is essential for the PR's objective of enabling cross-platform development and execution. The placement and commenting are clear and well-organized.
pkgs/edge-worker/src/platform/deno-types.ts (1)
1-20: Well-structured type definitions for cross-platform supportThe type definitions are cleanly implemented to facilitate Node.js development while ensuring Deno compatibility at runtime. The approach of defining only the minimal necessary types is appropriate for this use case.
pkgs/edge-worker/src/core/WorkerState.ts (2)
1-1: LGTM: Appropriate type import for LoggerThe import statement correctly imports the Logger type from the platform types module.
39-44: Good implementation of dependency injection for loggingThe refactoring from static logger initialization to dependency injection is a good practice that improves testability and aligns with the platform adapter approach.
pkgs/edge-worker/src/queue/Queue.ts (3)
4-4: LGTM: Appropriate type import for LoggerThe import statement correctly imports the Logger type from the platform types module.
7-15: Good implementation of dependency injection for loggingConverting to the dependency injection pattern for the logger is consistent with other changes and improves testability.
22-22: Comprehensive logging improves observabilityThe addition of debug logging statements to all significant operations provides excellent observability for troubleshooting. The messages include relevant context like queue names and message IDs.
Also applies to: 36-37, 46-47, 53-54, 60-61, 73-74, 99-100
pkgs/edge-worker/src/queue/MessageExecutor.ts (2)
4-4: LGTM: Appropriate type import for LoggerThe import statement correctly imports the Logger type from the platform types module.
22-22: Good implementation of dependency injection for loggingThe refactoring from static logger initialization to dependency injection aligns with the platform adapter approach and is consistent with other changes in the codebase.
Also applies to: 32-36
pkgs/edge-worker/src/core/Worker.ts (2)
3-3: Import of Logger type supports platform-agnostic loggingThe import of the Logger interface from platform/types.js rather than using a concrete implementation aligns with the Platform Adapter pattern, enabling the Worker to function consistently across different JavaScript runtimes.
7-7: Logger dependency injection improves testability and platform flexibilityRefactoring the logger to be injected through the constructor rather than being created internally supports:
- Better testability as the logger can be mocked during unit tests
- Platform-agnostic implementation as the logger implementation can vary by platform
- Consistent logging configuration across components
This change is critical for enabling the application to run in both Node.js and Deno environments.
Also applies to: 16-18, 22-22
pkgs/edge-worker/src/queue/ReadWithPollPoller.ts (3)
4-4: Logger type import supports platform abstractionThe import of the Logger interface from platform/types.js aligns with the Platform Adapter pattern being implemented.
14-23: Constructor injection of logger improves modularity and platform compatibilityThe constructor now accepts a Logger instance rather than creating one internally, following the dependency injection pattern that's being applied across the codebase. This enables the same code to run in both Node.js and Deno environments with different logger implementations.
27-28: Added debug logging enhances observabilityThe added logging statements at key points in the polling process provide valuable insights for debugging:
- When polling is aborted
- When starting to poll, including queue name and batch size
- After receiving messages, showing count of messages received
These are excellent additions for operational visibility.
Also applies to: 31-32, 38-40
pkgs/edge-worker/src/core/Heartbeat.ts (2)
3-3: Logger import enables platform abstractionImporting the Logger type from platform/types.js supports the Platform Adapter pattern being implemented.
6-6: Logger dependency injection supports platform independenceConverting the Heartbeat class to use constructor-injected logging rather than statically initialized logging:
- Follows the dependency injection pattern being applied throughout the codebase
- Enables the same code to run in different JavaScript runtimes with different logger implementations
- Improves testability by allowing the logger to be mocked in unit tests
This is a key refactoring for the Platform Adapter implementation.
Also applies to: 12-16
pkgs/edge-worker/src/flow/createFlowWorker.ts (4)
10-10: Logger type import enables platform abstractionImporting the Logger interface from platform/types.js supports the Platform Adapter pattern.
36-38: createLogger parameter implements flexible logging factory patternAdding the
createLoggerparameter to thecreateFlowWorkerfunction allows for platform-specific logger creation. This function signature change:
- Enables different logger implementations for Node.js and Deno
- Allows for contextual logging with module names
- Supports the overall Platform Adapter architecture
This is a key change for the cross-platform implementation.
39-39: Root logger initialization for module-level loggingCreating a module-specific logger for the factory function itself enables consistent logging across the application.
67-71: Consistent logger injection throughout component chainThe consistent pattern of creating named loggers for each component and passing them through constructors:
- Enables granular log filtering and reporting by component
- Maintains the dependency injection pattern across the entire codebase
- Supports platform independence by using the abstract Logger interface
This comprehensive implementation of the logging factory pattern is essential for the Platform Adapter's success.
Also applies to: 81-83, 90-96, 105-107, 113-115, 118-123
pkgs/edge-worker/src/platform/types.ts (3)
1-10: Well-designed Logger interface with appropriate log levels.The Logger interface provides a comprehensive set of logging methods covering the standard log levels (debug, info, warn, error). The interface is properly typed to accept additional arguments, which enables flexible logging patterns.
12-20: Good use of function type definitions for dependency injection.The
CreateLoggerFnandCreateWorkerFntypes are well-defined for dependency injection. This allows for more flexible and testable code by decoupling the creation of loggers and workers from their usage.
32-51: Well-structured PlatformAdapter interface with clear documentation.The PlatformAdapter interface is well-designed with appropriate methods for platform operations. The JSDoc comments are clear and informative, explaining the purpose of each method.
pkgs/edge-worker/src/queue/createQueueWorker.ts (4)
12-12: Properly importing Logger type from platform module.The type import ensures type safety when working with loggers throughout this module.
90-97: Good addition of createLogger parameter with appropriate documentation.The function signature has been properly updated to include the createLogger parameter, with corresponding JSDoc updates.
100-103: Effective use of component-specific logger.Creating a dedicated logger for the QueueWorker component with appropriate logging improves traceability and debugging.
115-119: Consistent application of logger dependency injection across all components.All component instantiations have been updated to include appropriate loggers created with descriptive module names. This consistent approach ensures proper logging context throughout the application's execution flow.
Also applies to: 123-127, 129-139, 141-151, 153-160, 162-167, 169-174
pkgs/edge-worker/src/core/BatchProcessor.ts (2)
3-6: Proper type import and field declaration for logger.The Logger type is correctly imported and the field is properly declared with explicit typing.
8-18: Well-updated constructor with logger parameter.The constructor now accepts a Logger instance and properly initializes the field, supporting the dependency injection pattern.
pkgs/edge-worker/src/core/ExecutionController.ts (2)
3-10: Proper type import and field declaration for logger.The Logger type is correctly imported and the field is properly declared with explicit typing.
15-25: Well-updated constructor with logger parameter.The constructor now accepts a Logger instance and properly initializes the field, supporting the dependency injection pattern.
pkgs/edge-worker/src/EdgeWorker.ts (5)
6-11: Imports look aligned with new adapter approach.
The introduction ofcreateAdapterand related types is cohesive and logically placed.
77-82: Ensure robust error handling when starting.
IfcreateAdapter()fails or returns a malfunctioning adapter, the error will bubble up without a catch. This might be acceptable depending on overarching design, but consider whether to handle it gracefully if partial initialization is a concern.🧰 Tools
🪛 Biome (1.9.4)
[error] 81-81: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
103-105: Good modular approach for worker creation.
Passing a logger factory is aligned with typical dependency injection patterns and fosters testability.
114-122: Stop method logic looks solid.
Raising an error ifstart()was not called ensures consistent lifecycle usage. If you anticipate erroneous calls from client code, you might log a note before throwing.🧰 Tools
🪛 Biome (1.9.4)
[error] 118-118: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 119-119: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
132-132: Removed method reference.
Comment states this block is “no longer needed”; if any references remain in external code, ensure they are cleaned up.pkgs/edge-worker/src/platform/DenoAdapter.ts (6)
17-26: Environmental guard in constructor is helpful.
Throwing an error whentypeof Deno === 'undefined'ortypeof EdgeRuntime === 'undefined'clarifies usage constraints.
55-60: Terminate method is straightforward.
Explicitly resettingthis.workerto null is clean. For thorough resource management, confirm if any open connections or timers also need stopping.
62-65: Throws error if adapter not yet initialized.
Clear message ensures consumers callinitialize()first. This ordering helps avoid partial usage.
67-109: Logger implementation is clear and flexible.
The dynamic log-level gating based on environment variables is good. If performance or formatting enhancements are needed, consider structured logs or external libraries.
146-166: Environment detection is thorough.
Good job verifying bothEDGE_WORKER_DB_URLandSB_EXECUTION_ID. The error messages guide users to configuration docs.
172-183: Graceful shutdown logic.
CallingspawnNewEdgeFunctionbefore unloading is an interesting choice for re-spawning. Make sure the environment expects new function instances upon shutdown.pkgs/dsl/tsconfig.lib.json (1)
6-7: Adding Node.js types enhances TypeScript compiler supportThis addition of Node.js type definitions is appropriate and aligns with the PR objective of enabling development on Node.js while allowing execution on Deno. The TypeScript compiler will now include Node.js type definitions during compilation, improving type checking and autocompletion for Node.js APIs.
pkgs/example-flows/tsconfig.lib.json (1)
6-7: Adding Node.js types enhances TypeScript compiler supportThe addition of Node.js type definitions is consistent with the changes made to other configuration files and supports the goal of enabling development in Node.js. This ensures that the example-flows package will have proper type checking when using Node.js APIs.
pkgs/edge-worker/src/flow/StepTaskPoller.ts (3)
3-3: Logger type import is properly updatedThe import change from a concrete logger implementation to a type import is a good step toward dependency injection and platform independence.
17-17: Logger is properly declared as an interface typeConverting the logger from a direct initialization to a typed property enables dependency injection and improves testability.
22-26: Constructor updated correctly to support logger injectionThe constructor now properly accepts a Logger parameter and initializes the logger property. This change:
- Improves testability by allowing mock loggers to be injected
- Enhances flexibility in configuring logging behavior
- Aligns with dependency injection best practices
This refactoring is consistent with the PR's goal of creating a platform-agnostic implementation.
pkgs/edge-worker/src/flow/StepTaskExecutor.ts (3)
4-4: Good addition of the Logger type import.This import correctly brings in the Logger interface from the platform-specific types, supporting the platform adapter pattern.
18-18: Good refactor to dependency injection for logging.Converting the logger from static initialization to a class property that can be injected improves testability and flexibility.
24-28:❓ Verification inconclusive
Constructor signature change needs to be propagated across the codebase.
The constructor has been updated to accept a Logger parameter, which is a good change for dependency injection. Make sure all instantiations of StepTaskExecutor are updated to pass this additional parameter.
🏁 Script executed:
#!/bin/bash # Find all places where StepTaskExecutor is instantiated rg -A 2 -B 2 "new StepTaskExecutor" --type typescriptLength of output: 90
Below is a revised review comment that requests manual verification based on the updated search procedure. Note that the original search using the
--type typescriptflag failed due to unsupported file types. Please re-run the search with a correct file glob (e.g.,-g "*.ts") to verify that all instantiations ofStepTaskExecutorproperly pass the newLoggerparameter.
Action Required: Propagate Constructor Signature Change
The constructor update to include a
Loggerparameter is a good move for dependency injection. However, ensure that every instantiation ofStepTaskExecutorin the codebase is updated accordingly. Since the initial search using--type typescriptproduced an error, please re-run the search using a command like the one below to cover all TypeScript files:After executing this updated search, review each instantiation to confirm that the additional
Loggerparameter is supplied. If any instantiations are found that have not been updated, please propagate the change to include the new argument.
🏁 Script executed:
#!/bin/bash # Find all instances where StepTaskExecutor is instantiated in TypeScript files rg -A 2 -B 2 "new StepTaskExecutor" -g "*.ts"Length of output: 509
Action Required: Verify and Update All Instantiations of StepTaskExecutor
The constructor for
StepTaskExecutornow requires an additionalLoggerparameter. Our recent search inpkgs/edge-worker/src/flow/createFlowWorker.tsrevealed an instantiation that only passes two arguments—likely omitting the newLoggerparameter:return new StepTaskExecutor<TFlow>( flow, record, // Missing Logger parameter? )Please re-run the verification with extended context if necessary and ensure all instantiation points across the codebase are updated to pass the new
Loggerparameter.pkgs/edge-worker/src/core/WorkerLifecycle.ts (3)
6-6: Good addition of the Logger type import.This import correctly brings in the Logger interface from the platform-specific types, supporting the platform adapter pattern.
13-14: Good refactoring of state and logger properties.Changing the initialization pattern for workerState and logger properties follows good dependency injection practices.
Also applies to: 15-15
38-38: Good update to Heartbeat initialization.The Heartbeat constructor now receives the logger parameter, maintaining consistent dependency injection throughout the application.
pkgs/edge-worker/src/flow/FlowWorkerLifecycle.ts (4)
4-4: Good addition of the Logger type import.This import correctly brings in the Logger interface from the platform-specific types, supporting the platform adapter pattern.
12-14: Good refactoring of state and logger properties.Changing the initialization pattern for workerState and logger properties follows good dependency injection practices.
34-39: Good update to Heartbeat initialization with improved formatting.The Heartbeat constructor now receives the logger parameter with better formatting for readability. This maintains consistent dependency injection throughout the application.
19-24:✅ Verification successful
Constructor signature change needs to be propagated.
The constructor has been updated to accept a Logger parameter. Ensure all instantiations of FlowWorkerLifecycle are updated to pass this additional parameter.
🏁 Script executed:
#!/bin/bash # Find all places where FlowWorkerLifecycle is instantiated rg -A 2 -B 2 "new FlowWorkerLifecycle" --type typescriptLength of output: 93
Ensure All FlowWorkerLifecycle Instantiations Pass the New Logger Parameter
The constructor for FlowWorkerLifecycle now accepts a Logger parameter. We need to verify that every instantiation of FlowWorkerLifecycle throughout the codebase has been updated accordingly. Since the initial ripgrep command failed due to an unrecognized file type, please run the following revised command to manually confirm the changes:
Once you confirm that all instantiations correctly pass the Logger, the code is good to go.
🏁 Script executed:
#!/bin/bash rg -A 2 -B 2 "new FlowWorkerLifecycle" -g "*.ts"Length of output: 589
I'm going to execute an additional shell script to inspect the surrounding context in the file
pkgs/edge-worker/src/flow/createFlowWorker.tsto verify if a Logger parameter is passed when constructing a new FlowWorkerLifecycle.Please run this command and share the output so we can confirm whether the Logger parameter is being correctly passed.
🏁 Script executed:
#!/bin/bash sed -n '/new FlowWorkerLifecycle/,+5p' pkgs/edge-worker/src/flow/createFlowWorker.tsLength of output: 329
Constructor Signature Update Confirmed
The update to include the Logger parameter in the FlowWorkerLifecycle constructor is correctly propagated. Verification in
pkgs/edge-worker/src/flow/createFlowWorker.tsconfirms that the logger is passed viacreateLogger('FlowWorkerLifecycle'). No other instantiations appear to be affected.
Refactor project structure and update package metadata: - Remove mod.ts file - Update package.json export paths to use dist directory - Add new npm dependency @teidesu/deno-types - Simplify deno.test.json lint and publish configurations - Update deno.lock with dependency changes
Remove supabase from root package.json Update dependencies in edge-worker package.json Reorder and synchronize package dependency versions
Deploying pgflow with
|
| Latest commit: |
c65f6ee
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://02b763a1.pgflow.pages.dev |
| Branch Preview URL: | https://platform-adapter.pgflow.pages.dev |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
pkgs/edge-worker/deno.test.json (2)
1-14: Removal of the "exports" Field
The configuration no longer defines an"exports"field. This change aligns with the new module resolution strategy for Deno and is consistent with the overall platform adapter goal. Please verify that none of the downstream tooling or consumers rely on this field for proper resolution.
30-33: Refined Publish Configuration
The"publish"configuration now includes["README.md", "LICENSE.md", "CHANGELOG.md", "src/**/*.ts"]and explicitly excludes"__tests__/**/*". This streamlining should help maintain a clean package. Please confirm that no necessary files (such as entry points likemod.tsif used elsewhere) are unintentionally excluded from the published package.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pkgs/edge-worker/deno.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
package.json(0 hunks)pkgs/edge-worker/deno.test.json(1 hunks)pkgs/edge-worker/mod.ts(0 hunks)pkgs/edge-worker/package.json(1 hunks)
💤 Files with no reviewable changes (2)
- package.json
- pkgs/edge-worker/mod.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- pkgs/edge-worker/package.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: main
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
pkgs/edge-worker/deno.test.json (1)
24-29: Updated Lint Configuration
The"lint"section now excludes["supabase/functions/", "dist/", "node_modules/"]. The removal of the specific exclusion for"supabase/functions/_src/"simplifies the configuration. Ensure that the new exclusion list still covers all the undesired paths and does not inadvertently miss any folder that might require linting exclusion.
Rationale: - The diff shows a removal of the "pino" package from package.json dependencies - This is a dependency management change, which falls under the 'chore' type - The description succinctly captures the specific action (removing an unused dependency) - The scope '(deps)' indicates it's related to package dependencies - The message is clear and follows Conventional Commits standard The commit message meets all the specified guidelines: - Uses appropriate type (chore) - Includes an optional scope (deps) - Provides a clear, concise description of the change - Follows the <type>[optional scope]: <description> format
Modify build and import paths for Supabase edge functions - Add build:e2e executor configuration in project.json - Update .gitignore to include _dist folder - Update import paths in edge function files from dist to _dist - Remove pino dependency from deno.lock - Add Supabase dependency to project configuration - Update e2e test configuration to depend on new build step
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkgs/edge-worker/project.json (1)
17-26: New Build E2E Target ConfigurationThe addition of the
build:e2etarget using the@nx/esbuild:esbuildexecutor is a strong move for performance and compatibility. The configuration—especially the bundling settings and ESM format—is well-aligned for deploying to environments like Deno. Please verify that the output directory (supabase/functions/_dist) fits your deployment strategy and that the TypeScript configuration is compatible with both Node.js and Deno environments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pkgs/edge-worker/deno.lockis excluded by!**/*.lockpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
pkgs/edge-worker/.gitignore(1 hunks)pkgs/edge-worker/package.json(1 hunks)pkgs/edge-worker/project.json(2 hunks)pkgs/edge-worker/supabase/functions/cpu_intensive/index.ts(1 hunks)pkgs/edge-worker/supabase/functions/max_concurrency/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- pkgs/edge-worker/.gitignore
- pkgs/edge-worker/supabase/functions/max_concurrency/index.ts
- pkgs/edge-worker/supabase/functions/cpu_intensive/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- pkgs/edge-worker/package.json
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (1)
pkgs/edge-worker/project.json (1)
153-163: Test E2E Target Update: Dependency on Build E2EUpdating the
test:e2etarget to depend on bothsupabase:prepare-edge-fnand the newbuild:e2etarget ensures that end-to-end tests run against the most recent build output. This change enhances the reliability of the test process. Please double-check that the dependency chain does not affect parallel execution or introduce unnecessary build overhead in your CI/CD pipeline.
Refactored DenoAdapter to enhance type safety and initialization process: - Removed unnecessary null checks and simplified worker management - Updated worker initialization with explicit start method - Removed deprecated setWorker and spawnNewEdgeFunction methods - Strengthened type constraints for environment and worker properties
…nction setup Simplify build and deployment processes by: - Removing redundant Supabase edge function preparation steps - Updating project.json build and dependency configurations - Modifying DenoAdapter to use internal edge function name - Adding Deno configuration files for cpu_intensive and max_concurrency functions
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
pkgs/edge-worker/project.json (1)
17-26: New Build Bundle Target AddedThe addition of the
"build:bundle"target is well implemented. It correctly specifies the primary build options, including the main entry point, output path, TypeScript configuration, bundle flag, and output format. One improvement to consider (optional) is evaluating if additional esbuild options (such as target environment settings or minification) might further optimize production builds.pkgs/edge-worker/src/platform/DenoAdapter.ts (3)
19-30: Consider using a factory pattern for platform-specific adaptersThe error message suggests that this error is "expected during build only", which might be confusing. A better approach might be to use a factory pattern that checks the environment first.
This would also align better with the PR's objective of enabling development on Node.js while allowing execution on Deno.
70-77: Remove unnecessary optional chainingThe optional chaining on
this.env?.executionIdandthis.env?.logLevelsuggests thatthis.envmight be undefined, but it should be defined in the constructor.createLogger(module: string): Logger { - const workerId = this.env?.executionId || 'unknown'; - const logLevel = this.env?.logLevel || 'info'; + const workerId = this.env.executionId || 'unknown'; + const logLevel = this.env.logLevel || 'info'; // Simple level filtering const levels = { error: 0, warn: 1, info: 2, debug: 3 }; const levelValue = levels[logLevel as keyof typeof levels] ?? levels.info;
167-169: Add validation for extracted function nameThe current implementation doesn't validate the extracted function name. It could be empty if the URL pathname is
/.private extractFunctionName(req: Request): string { - return new URL(req.url).pathname.replace(/^\/+|\/+$/g, ''); + const name = new URL(req.url).pathname.replace(/^\/+|\/+$/g, ''); + if (!name) { + throw new Error('Function name could not be extracted from URL path'); + } + return name; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
pkgs/edge-worker/supabase/functions/cpu_intensive/deno.lockis excluded by!**/*.lockpkgs/edge-worker/supabase/functions/max_concurrency/deno.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
pkgs/edge-worker/.gitignore(1 hunks)pkgs/edge-worker/project.json(3 hunks)pkgs/edge-worker/src/platform/DenoAdapter.ts(1 hunks)pkgs/edge-worker/supabase/functions/cpu_intensive/deno.json(1 hunks)pkgs/edge-worker/supabase/functions/max_concurrency/deno.json(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- pkgs/edge-worker/supabase/functions/cpu_intensive/deno.json
- pkgs/edge-worker/supabase/functions/max_concurrency/deno.json
🚧 Files skipped from review as they are similar to previous changes (1)
- pkgs/edge-worker/.gitignore
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkgs/edge-worker/src/platform/DenoAdapter.ts (2)
pkgs/edge-worker/src/platform/types.ts (4)
PlatformAdapter(35-51)PlatformEnvironment(25-30)Logger(5-10)CreateWorkerFn(20-20)pkgs/edge-worker/src/core/Worker.ts (1)
Worker(5-100)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (4)
pkgs/edge-worker/project.json (2)
101-110: Enhanced Dependency in supabase:functions-serveUpdating the
"supabase:functions-serve"target to depend on both"supabase:start"and"build:bundle"ensures that the bundled assets are freshly generated before serving functions. This change aligns well with the platform adapter update and overall build process.
141-151: Updated Dependency in test:e2e TargetModifying the
"test:e2e"target so it depends on"build:bundle"is a solid approach to guarantee that end-to-end tests run against the latest bundle. This helps prevent test failures due to outdated or missing build artifacts.pkgs/edge-worker/src/platform/DenoAdapter.ts (2)
1-9: Good use of type imports and module structureThe imports are well organized and appropriately use
typeimports for TypeScript types. The module structure follows good practices.
145-165: Well-structured environment detection with good error messagesThe
detectEnvironmentmethod is well-structured and provides clear, informative error messages when required environment variables are missing.
Add shutdown handler and logger initialization Simplify worker termination process by extracting terminate method
Deleted redundant comment in EdgeWorker class
Add @teidesu/deno-types dependency to project dependencies and test configuration
Introduce createFakeLogger for consistent test logging across integration tests. Update multiple test files to use the new fake logger when creating queue and flow workers, improving test isolation and reducing console noise
Suppress eslint no-empty-function warning in test utilities
Modify project.json to add rootDir configuration Update index.ts to export from .js file extension instead of .ts
Remove redundant JSON file rules and dependency checks from package-specific ESLint configs, using only the base configuration
Update GitHub Actions workflow to use specific versions of Node.js and pnpm for consistent build environment
Bumped Deno version to 1.45.2 and added sqruff CLI installation step in GitHub workflow
Update type definition for Deno from "deno/full" to "deno/full.d.ts" in TypeScript configuration
Remove unnecessary import and update package.json dependency
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
pkgs/edge-worker/src/platform/DenoAdapter.ts (1)
45-49: 🛠️ Refactor suggestionAdd error handling when stopping the worker
If the.stop()call fails, it could cause an unhandled rejection. Wrap this in a try/catch to gracefully handle shutdown errors.async stopWorker(): Promise<void> { if (this.worker) { - await this.worker.stop(); + try { + await this.worker.stop(); + } catch (error) { + this.logger.error(`Failed to stop worker: ${error}`); + } } }
🧹 Nitpick comments (4)
pkgs/edge-worker/src/platform/DenoAdapter.ts (4)
28-29: Unreachable fallback for log level
BecausegetEnvVarOrThrow()throws if the environment variable is missing, the fallback'info'is unreachable. Either remove the fallback or switch to a non-throwing method if you wish to default to'info'.- const logLevel = this.getEnvVarOrThrow('EDGE_WORKER_LOG_LEVEL') || 'info'; + const envLogLevel = Deno.env.get('EDGE_WORKER_LOG_LEVEL'); + const logLevel = envLogLevel && envLogLevel.trim() !== '' + ? envLogLevel + : 'info';
100-105: Reorder success log after checking response status
"Edge Function spawned successfully!" is logged before confirming the response is OK. Log success only after verifyingresponse.ok.this.logger.debug('Spawning a new Edge Function...'); const response = await fetch(/* ... */); - this.logger.debug('Edge Function spawned successfully!'); - if (!response.ok) { throw new Error( /* ... */ ); } + this.logger.debug('Edge Function spawned successfully!');
113-123: Prevent shutdown failures due to spawn errors
IfspawnNewEdgeFunction()fails during shutdown, the entire shutdown may be aborted. Consider wrapping this in a try/catch to ensure shutdown completes despite spawn errors.globalThis.onbeforeunload = async () => { this.logger.info('Shutting down...'); if (this.worker) { - await this.spawnNewEdgeFunction(); + try { + await this.spawnNewEdgeFunction(); + } catch (error) { + this.logger.error(`Error spawning new Edge Function on shutdown: ${error}`); + } } await this.stopWorker(); };
140-141: Log the function name after assignment
Currently,this.edgeFunctionNameis logged before it's assigned for the first request. Move the log statement after retrieving the function name to avoid loggingnull.Deno.serve({}, (req: Request) => { - this.logger.info(`HTTP Request: ${this.edgeFunctionName}`); if (!this.worker) { this.edgeFunctionName = this.extractFunctionName(req); /* ... */ } + this.logger.info(`HTTP Request: ${this.edgeFunctionName}`); return new Response(/* ... */); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
pkgs/edge-worker/package.json(1 hunks)pkgs/edge-worker/src/platform/DenoAdapter.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pkgs/edge-worker/package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkgs/edge-worker/src/platform/DenoAdapter.ts (3)
pkgs/edge-worker/src/platform/types.ts (3)
PlatformAdapter(25-41)Logger(5-10)CreateWorkerFn(20-20)pkgs/edge-worker/src/core/Worker.ts (1)
Worker(5-100)pkgs/edge-worker/src/platform/logging.ts (1)
createLoggingFactory(6-91)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: main
- GitHub Check: Cloudflare Pages
Refactor TypeScript configuration for edge-worker package: - Update package.json dependency to specific @teidesu/deno-types version - Add new tsconfig.json with project references - Simplify tsconfig.lib.json by extending new configuration - Add edge-worker path to root tsconfig.json project references
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkgs/edge-worker/tsconfig.lib.json (1)
4-11: Streamlined Compiler OptionsThe removal of the local
compilerOptionsblock (which previously includedrootDir,outDir, andtsBuildInfoFile) simplifies the configuration. Ensure that the new./tsconfig.jsonreferenced in line 2 now correctly provides these options. This change contributes to a cleaner, more maintainable configuration structure for the edge-worker package.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
pkgs/edge-worker/package.json(1 hunks)pkgs/edge-worker/tsconfig.json(1 hunks)pkgs/edge-worker/tsconfig.lib.json(1 hunks)tsconfig.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pkgs/edge-worker/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (1)
- pkgs/edge-worker/package.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: main
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
tsconfig.json (1)
12-17: Edge-Worker Reference AddedThe new entry for
"./pkgs/edge-worker"in the"references"array is correctly introduced. This update ensures that the Edge Worker package is now part of the TypeScript project references, aligning with the PR's goal of enhancing multi-package integration.pkgs/edge-worker/tsconfig.lib.json (1)
1-3: Localized Configuration InheritanceChanging the
"extends"property from"../../tsconfig.base.json"to"./tsconfig.json"localizes the configuration for the edge-worker package. This is a good move as it allows the package to maintain its own TypeScript settings while still referencing shared configuration settings via its localtsconfig.json.
Reordered and updated package dependencies in package.json
Add traceResolution flag to project build configuration to improve module resolution diagnostics
Migrate Deno type references from @teidesu/deno-types to native deno types Update package.json and tsconfig to use standard Deno type definitions
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
pkgs/edge-worker/src/platform/DenoAdapter.ts (2)
81-103: Log “Edge Function spawned successfully!” after verifying response is OK.Currently, you log success at line 102 before checking
response.okat lines 104-108, which can be misleading if the fetch ultimately fails. This was noted in past reviews.Applying the suggestion from previous feedback, move the success log after verifying the response is OK:
- this.logger.debug('Edge Function spawned successfully!'); if (!response.ok) { throw new Error( ... ); } + this.logger.debug('Edge Function spawned successfully!');
115-124: Add error handling for spawnNewEdgeFunction in shutdown flow.If
spawnNewEdgeFunction()fails in theonbeforeunloadhandler, the entire function may reject before callingstopWorker(). This was flagged in a past review.Wrap the call in try/catch to gracefully handle errors:
if (this.worker) { - await this.spawnNewEdgeFunction(); + try { + await this.spawnNewEdgeFunction(); + } catch (error) { + this.logger.error(`Failed to spawn new edge function: ${error}`); + } }
🧹 Nitpick comments (2)
pkgs/edge-worker/src/platform/DenoAdapter.ts (2)
1-10: Consider adding dedicated tests for DenoAdapter.This file is entirely new and appears to have no corresponding test suite. This might undermine confidence in its stability and introduce untested logic paths.
Would you like help in creating a unit test suite for the new adapter, or opening a follow-up task to track this?
30-30: Remove unreachable log level fallback.Since
getEnvVarOrThrow('EDGE_WORKER_LOG_LEVEL')will throw ifEDGE_WORKER_LOG_LEVELis unset, the|| 'info'fallback is never used. Consider allowing the variable to be optional or removing the fallback.Here is a snippet that makes the variable optional while warning if not set:
- const logLevel = this.getEnvVarOrThrow('EDGE_WORKER_LOG_LEVEL') || 'info'; + const levelFromEnv = Deno.env.get('EDGE_WORKER_LOG_LEVEL'); + const logLevel = levelFromEnv ?? 'info'; + if (!levelFromEnv) { + console.warn('EDGE_WORKER_LOG_LEVEL is not set; defaulting to "info"'); + } this.loggingFactory.setLogLevel(logLevel);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
pkgs/edge-worker/package.json(1 hunks)pkgs/edge-worker/src/platform/DenoAdapter.ts(1 hunks)pkgs/edge-worker/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkgs/edge-worker/tsconfig.json
- pkgs/edge-worker/package.json
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkgs/edge-worker/src/platform/DenoAdapter.ts (3)
pkgs/edge-worker/src/platform/types.ts (3)
PlatformAdapter(25-41)Logger(5-10)CreateWorkerFn(20-20)pkgs/edge-worker/src/core/Worker.ts (1)
Worker(5-100)pkgs/edge-worker/src/platform/logging.ts (1)
createLoggingFactory(6-91)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
| private setupStartupHandler(createWorkerFn: CreateWorkerFn): void { | ||
| Deno.serve({}, (req: Request) => { | ||
| this.logger.info(`HTTP Request: ${this.edgeFunctionName}`); | ||
|
|
||
| if (!this.worker) { | ||
| this.edgeFunctionName = this.extractFunctionName(req); | ||
|
|
||
| const workerId = this.getEnvVarOrThrow('SB_EXECUTION_ID'); | ||
|
|
||
| this.loggingFactory.setWorkerId(workerId); | ||
|
|
||
| // Create the worker using the factory function and the logger | ||
| this.worker = createWorkerFn(this.loggingFactory.createLogger); | ||
| this.worker.startOnlyOnce({ | ||
| edgeFunctionName: this.edgeFunctionName, | ||
| workerId, | ||
| }); | ||
| } | ||
|
|
||
| return new Response('ok', { | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential concurrency issue on startup.
When multiple requests arrive concurrently and this.worker is still null, multiple threads of execution may try to create a worker. Though worker.startOnlyOnce() helps mitigate repeated starts, consider confirming there are no race conditions.
Run the following script to see if concurrency guard patterns exist or if additional locking is needed:
🏁 Script executed:
#!/bin/bash
# Search for concurrency guards in the code referencing the Worker
rg -A 5 "this\.worker.*= createWorkerFn"Length of output: 511
Concurrency Initialization Concern: Review for Proper Synchronization
The current implementation’s check (if (!this.worker)) combined with worker.startOnlyOnce does not guarantee atomicity. The search confirms that no additional concurrency guards (e.g., locks or mutexes) are present. This could lead to multiple concurrent invocations entering the initialization block before this.worker is set.
- Potential Issue: Multiple requests arriving concurrently might trigger parallel worker creation, even though
startOnlyOnceprevents redundant starts. - Suggestion: Consider implementing an explicit concurrency guard (such as a mutex or an initialization promise) to ensure that only one thread can create the worker when
this.workeris null. - Action: Review and adjust the startup logic in
setupStartupHandler(lines 140–163 ofpkgs/edge-worker/src/platform/DenoAdapter.ts) to synchronize worker initialization.
Remove traceResolution from build options and downgrade @types/deno package version
Adjust TypeScript configuration for Deno compatibility: - Rename deno-types.ts to deno-types.d.ts - Update tsconfig.json types to include specific Deno type definitions - Remove unnecessary import in DenoAdapter.ts
Add type references for Deno environment and remove unnecessary import
Remove Deno-specific type definitions, keeping only Node types
Update @teidesu/deno-types dependency to version 1.45.2 and refactor TypeScript configuration to include Deno-specific type definitions
Add Deno full type reference and include custom Deno type definitions in edge-worker TypeScript configuration
Update package.json and tsconfig.json to improve type definitions and dependency management for edge worker package
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkgs/edge-worker/tsconfig.json (3)
1-4: Configuration Basics ReviewThe top-level properties—including
"extends","files", and"include"—are defined as intended. However, note that both"files"and"include"are empty. Make sure this aligns with your build strategy (perhaps relying solely on project references) or if source file patterns should be explicitly specified.
5-16: Compiler Options SetupThe
compilerOptionsare configured appropriately with"rootDir","outDir", and"tsBuildInfoFile"defined for proper directory management. The"typeRoots"and"types"settings reflect the dual Node.js and Deno targets and include local type definitions (e.g.,"src/platform/deno-types.d.ts"). Please verify that the relative paths for the Deno type definitions (such as"deno/ext/lib.deno.fetch.d.ts"and"deno/ext/lib.deno.window.d.ts") correctly resolve in your project structure.
17-24: Project References ConfigurationThe
"references"section correctly includes paths to"../dsl"and"../core", which is useful for managing project dependencies. Ensure that these paths are correct and that the referenced projects are compatible with the updated configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
pkgs/edge-worker/package.json(1 hunks)pkgs/edge-worker/tsconfig.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: main
- GitHub Check: Cloudflare Pages
🔇 Additional comments (3)
pkgs/edge-worker/package.json (3)
12-13: Verify Correct Distribution Paths in ExportsThe updated
"types"and"import"paths now correctly point to the files in thedistfolder, aligning with the new build output structure. This ensures that both type checking and module resolution function as intended.
22-23: Confirm Dependency Reordering and VersioningThe reordering within the
"dependencies"section—placing"@pgflow/dsl"before"postgres"—improves readability and logical grouping. While JSON order does not affect dependency resolution, please verify that the versions and workspace references remain correct.
25-28: Review and Validate New DevDependenciesThe addition of a new
"devDependencies"section with:
"@types/deno": "npm:@teidesu/[email protected]","@types/node": "~18.16.9","supabase": "2.21.1",supports enhanced TypeScript configuration and development support. Ensure these versions meet project compatibility requirements and align with the overall dependency strategy.
This PR introduces Platform Adapter interface.
This is required to be able to build on Node.js but run on Deno.
Will be extremely helpful in porting Edge Worker to other platforms, but the main motivation
was that I started Edge Worker in Deno, but it is easier to develop and build it in Node becuase it pulls other libraries from @pgflow scope.
Summary by CodeRabbit
New Features
Refactor
Chores
Tests