-
Notifications
You must be signed in to change notification settings - Fork 9
608 run set up scripts before loading modules #638
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?
Conversation
## Walkthrough
This update introduces a new `ResourceManager` to manage setup and teardown scripts, restructures server and worker lifecycle management to coordinate resource initialization before loading modules, and refactors sourcing and file management into local and remote variants. Several new interfaces and classes are added for modularity, and the startup sequence is refactored to ensure resources are ready before module loading.
## Changes
| Files/Paths | Change Summary |
|-----------------------------------------------------------------------------------------------------|----------------|
| `packages/sourcing/src/files/LocalFileManager.ts`, `RemoteFileManager.ts`, `errors/RemoteFilesNotSupported.ts`, `index.ts`, `interfaces/FileReader.ts` | Added new file manager classes for local/remote files, a new error class, a file reader interface, and a file module index. |
| `packages/sourcing/src/modules/ImportManager.ts`, `LocalImportManager.ts`, `RemoteImportManager.ts`, `LocalModuleLocator.ts`, `RemoteModuleLocator.ts`, `interfaces/ModuleImporter.ts`, `interfaces/ModuleLocator.ts`, `index.ts` | Added new import manager and module locator classes for local/remote, new interfaces, and a module index. |
| `packages/sourcing/src/SourcingManager.ts`, `LocalSourcingManager.ts`, `RemoteSourcingManager.ts`, `index.ts` | Refactored sourcing manager to use injected file/import managers, added local/remote sourcing managers, and updated exports. |
| `packages/sourcing/src/files/FileManager.ts` | Refactored to require explicit file system injection and implement `FileReader`. |
| `packages/sourcing/test/fixtures/TestFileSystem.ts` | Updated import path for `LocalFileSystem`. |
| `packages/build/src/BuildManager.ts`, `packages/configuration/src/ConfigurationManager.ts`, `packages/cli/src/commands/StartServer.ts` | Switched to using `LocalFileManager` or `LocalSourcingManager` for file/sourcing management. |
| `packages/execution/package.json` | Added dependency on `@jitar/sourcing`. |
| `packages/execution/src/ExecutionManager.ts`, `models/Application.ts` | Added lifecycle methods for segment loading/unloading and clearing segments. |
| `packages/health/src/HealthManager.ts` | Added dynamic health check loading/unloading and lifecycle methods. |
| `packages/middleware/src/MiddlewareManager.ts` | Added dynamic middleware loading/unloading and lifecycle methods. |
| `packages/plugin-vite/src/index.ts` | Changed client bundle to explicitly start the client after build. |
| `packages/runtime/src/client/Client.ts` | Refactored startup/shutdown to coordinate worker and middleware manager, moving middleware setup to `start()`. |
| `packages/runtime/src/client/ClientBuilder.ts` | Refactored to inject `RemoteSourcingManager` into middleware and execution managers. |
| `packages/runtime/src/server/ResourceManager.ts` | Introduced `ResourceManager` to manage setup/teardown scripts. |
| `packages/runtime/src/server/Server.ts` | Refactored to use `ResourceManager` for resource lifecycle, changed startup order to start resources before loading modules. |
| `packages/runtime/src/server/ServerBuilder.ts` | Refactored to build and inject `ResourceManager`, removed async module loading from build steps. |
| `packages/services/src/worker/LocalWorker.ts` | Added `registerAtGateway` flag, refactored worker lifecycle to always start/stop execution manager, conditional gateway registration. |
| `packages/services/test/repository/fixtures/sourcingManager.fixture.ts` | Refactored fixture to use new sourcing manager structure and removed unused methods. |
| `examples/resources/package.json` | Changed package name. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant Server
participant ResourceManager
participant MiddlewareManager
participant HealthManager
participant ExecutionManager
Server->>ResourceManager: start()
ResourceManager->>ResourceManager: run setup scripts
ResourceManager-->>Server: setup complete
Server->>MiddlewareManager: start()
Server->>HealthManager: start()
Server->>ExecutionManager: start()
MiddlewareManager-->>Server: ready
HealthManager-->>Server: ready
ExecutionManager-->>Server: ready
Note over Server: Modules are loaded <br>after setup scripts complete
Server->>ResourceManager: stop()
ResourceManager->>ResourceManager: run teardown scripts
ResourceManager-->>Server: teardown complete Assessment against linked issues
Poem
|
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.
Actionable comments posted: 8
🧹 Nitpick comments (13)
packages/sourcing/src/files/interfaces/FileReader.ts (1)
4-11
: Consider adding JSDoc comments to the interface methodsThe
FileReader
interface is well-defined with clear method signatures, but adding JSDoc comments would improve the documentation and make the interface more self-explanatory for developers.interface FileReader { + /** + * Filters files based on the provided patterns. + * @param patterns - Glob patterns to match files against + * @returns Promise resolving to an array of matching filenames + */ filter(...patterns: string[]): Promise<string[]>; + /** + * Checks if a file exists at the specified path. + * @param filename - Path to the file to check + * @returns Promise resolving to a boolean indicating if the file exists + */ exists(filename: string): Promise<boolean>; + /** + * Reads the content of a file. + * @param filename - Path to the file to read + * @returns Promise resolving to a File object containing the file contents + */ read(filename: string): Promise<File>; }packages/sourcing/src/modules/ImportManager.ts (2)
22-23
: Consider expanding the comment to explain why awaiting is necessary.The comment correctly indicates that awaiting is necessary to catch errors, but it could be more educational by explaining the alternative behavior (errors propagating up the call stack) if
await
wasn't used here.
16-32
: Consider adding more granular error handling.The current error handling wraps all import errors into a
ModuleNotLoaded
error with the original message. You might want to distinguish between different types of errors (e.g., syntax errors, module not found, network errors for remote imports) to provide more specific error messages or handling strategies.packages/sourcing/src/modules/index.ts (1)
10-11
: Add newline at the end of the file.The file is missing a newline at the end, which is considered a best practice for text files and is enforced by many linters.
export { default as LocalImportManager } from './LocalImportManager'; export { default as RemoteImportManager } from './RemoteImportManager'; +
packages/sourcing/src/files/index.ts (1)
13-14
: Add newline at the end of the file.The file is missing a newline at the end, which is considered a best practice for text files and is enforced by many linters.
export { default as LocalFileManager } from './LocalFileManager'; export { default as RemoteFileManager } from './RemoteFileManager'; +
packages/runtime/src/server/ResourceManager.ts (2)
17-25
: Consider adding script execution status reporting.The start and stop methods could benefit from reporting the execution status of each script. This would be helpful for debugging and monitoring purposes, especially in production environments.
37-40
: Consider sequential execution option for scripts.Currently, all scripts are executed in parallel using Promise.all. For some scenarios, sequential execution might be preferred to ensure one script completes before the next begins. Consider adding an option to run scripts sequentially if ordered execution is important.
packages/sourcing/src/files/RemoteFileSystem.ts (2)
14-68
: Consistent error handling for unsupported operationsMost methods appropriately throw a RemoteFilesNotSupported error to clearly indicate that remote file operations are not currently supported.
However, there's an inconsistency with the filter() method returning an empty array instead of throwing an error. Consider whether this should be consistent with other methods.
- async filter(location: string, pattern: string): Promise<string[]> - { - return []; - } + filter(location: string, pattern: string): Promise<string[]> + { + throw new RemoteFilesNotSupported(); + }Alternatively, if there's a specific reason for filter() to return an empty array instead of throwing, consider adding a comment explaining this decision.
49-53
: Consider documenting resolve() method's behaviorThe resolve() method returns the input location unchanged, unlike most other methods that throw exceptions. Consider adding a comment explaining why this method has different behavior.
resolve(location: string): string { + // This method doesn't throw an error because it's needed for basic path handling + // even when remote file operations are not supported return location; }packages/runtime/src/server/Server.ts (2)
237-250
: Startup sequence may miss ProcedureRunner initialisation
middlewareManager.start()
is awaited beforeProcedureRunner
is created and added.
If the manager performs one-off initialisation (e.g. building a chain of middlewares) the newly addedProcedureRunner
will not be part of that setup.- await Promise.all( - [ - this.#proxy.start(), - this.#healthManager.start(), - this.#middlewareManager.start() - ]); - - const procedureRunner = new ProcedureRunner(this.#proxy); - this.#middlewareManager.addMiddleware(procedureRunner); + const procedureRunner = new ProcedureRunner(this.#proxy); + this.#middlewareManager.addMiddleware(procedureRunner); + + await Promise.all([ + this.#proxy.start(), + this.#healthManager.start(), + this.#middlewareManager.start() + ]);Placing the
ProcedureRunner
in the chain before the manager starts ensures consistent middleware ordering and avoids a second explicit “reload”.
252-262
: Graceful rollback on partial start / stop failures
#setUp()
and#tearDown()
execute several async tasks in parallel without a surroundingtry/catch
.
If one task fails, the server may end up in a half-started (or half-stopped) state, leaking resources.Consider:
- Wrapping the whole block in
try/catch
.- Implementing a best-effort rollback (e.g. stop already-started components).
- Logging the partial failure so that operators know which piece failed.
This will make the server startup sequence more resilient in production.
packages/runtime/src/server/ServerBuilder.ts (2)
137-157
: DRY: Extract common “ensure.js
” mapping
#buildResourceManager
,#buildHealthManager
, and#buildMiddlewareManager
repeat identical logic. A tiny helper keeps this file shorter and future-proof.+#translateFilenames(filenames: string[] = []): string[] { + return filenames.map(this.#assureExtension.bind(this)); +} const translatedSetUp = this.#translateFilenames(setUp); const translatedTearDown = this.#translateFilenames(tearDown); ... const translatedFilenames = this.#translateFilenames(filenames);Not urgent, but worthwhile as more managers appear.
25-42
: Optional: rename local variableproxy
for clarityWith the earlier aliasing of
ServiceProxy
, the localproxy
instance remains, but the name duplicates the concept.
ConsiderserviceProxy
to slightly improve readability:-const proxy = await this.#buildService(configuration); +const serviceProxy = await this.#buildService(configuration); ... -return new Server({ proxy, sourcingManager, remoteBuilder, resourceManager, middlewareManager, healthManager, logger }); +return new Server({ proxy: serviceProxy, sourcingManager, remoteBuilder, resourceManager, middlewareManager, healthManager, logger });Purely a readability nitpick—feel free to skip.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
examples/resources/package.json
(1 hunks)packages/build/src/BuildManager.ts
(2 hunks)packages/cli/src/commands/StartServer.ts
(2 hunks)packages/configuration/src/ConfigurationManager.ts
(2 hunks)packages/execution/package.json
(1 hunks)packages/execution/src/ExecutionManager.ts
(3 hunks)packages/execution/src/models/Application.ts
(1 hunks)packages/health/src/HealthManager.ts
(3 hunks)packages/middleware/src/MiddlewareManager.ts
(2 hunks)packages/plugin-vite/src/index.ts
(1 hunks)packages/runtime/src/client/Client.ts
(2 hunks)packages/runtime/src/client/ClientBuilder.ts
(2 hunks)packages/runtime/src/server/ResourceManager.ts
(1 hunks)packages/runtime/src/server/Server.ts
(3 hunks)packages/runtime/src/server/ServerBuilder.ts
(6 hunks)packages/services/src/worker/LocalWorker.ts
(4 hunks)packages/services/test/repository/fixtures/sourcingManager.fixture.ts
(2 hunks)packages/sourcing/src/LocalSourcingManager.ts
(1 hunks)packages/sourcing/src/RemoteSourcingManager.ts
(1 hunks)packages/sourcing/src/SourcingManager.ts
(2 hunks)packages/sourcing/src/files/FileManager.ts
(1 hunks)packages/sourcing/src/files/LocalFileManager.ts
(1 hunks)packages/sourcing/src/files/RemoteFileManager.ts
(1 hunks)packages/sourcing/src/files/RemoteFileSystem.ts
(1 hunks)packages/sourcing/src/files/errors/RemoteFilesNotSupported.ts
(1 hunks)packages/sourcing/src/files/index.ts
(1 hunks)packages/sourcing/src/files/interfaces/FileReader.ts
(1 hunks)packages/sourcing/src/index.ts
(1 hunks)packages/sourcing/src/modules/ImportManager.ts
(1 hunks)packages/sourcing/src/modules/LocalImportManager.ts
(1 hunks)packages/sourcing/src/modules/LocalModuleLocator.ts
(1 hunks)packages/sourcing/src/modules/RemoteImportManager.ts
(1 hunks)packages/sourcing/src/modules/RemoteModuleLocator.ts
(1 hunks)packages/sourcing/src/modules/index.ts
(1 hunks)packages/sourcing/src/modules/interfaces/ModuleImporter.ts
(1 hunks)packages/sourcing/src/modules/interfaces/ModuleLocator.ts
(1 hunks)packages/sourcing/test/fixtures/TestFileSystem.ts
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (10)
packages/configuration/src/ConfigurationManager.ts (1)
packages/sourcing/src/files/LocalFileManager.ts (1)
LocalFileManager
(5-11)
packages/sourcing/src/modules/RemoteImportManager.ts (3)
packages/sourcing/src/modules/LocalImportManager.ts (1)
LocalImportManager
(7-13)packages/sourcing/src/modules/ImportManager.ts (1)
ImportManager
(7-33)packages/sourcing/src/modules/RemoteModuleLocator.ts (1)
RemoteModuleLocator
(5-18)
packages/sourcing/src/files/interfaces/FileReader.ts (2)
packages/runtime/src/server/ServerBuilder.ts (2)
patterns
(166-173)filename
(175-180)packages/sourcing/src/files/models/File.ts (1)
File
(2-22)
packages/build/src/BuildManager.ts (1)
packages/sourcing/src/files/LocalFileManager.ts (1)
LocalFileManager
(5-11)
packages/sourcing/src/files/RemoteFileSystem.ts (1)
packages/sourcing/src/files/errors/RemoteFilesNotSupported.ts (1)
RemoteFilesNotSupported
(2-8)
packages/sourcing/src/SourcingManager.ts (4)
packages/sourcing/src/index.ts (6)
FileReader
(2-2)ModuleImporter
(5-5)FileManager
(3-3)ImportManager
(6-6)File
(3-3)Module
(5-5)packages/sourcing/src/files/FileManager.ts (1)
FileManager
(12-129)packages/sourcing/src/modules/ImportManager.ts (1)
ImportManager
(7-33)packages/sourcing/src/files/models/File.ts (1)
File
(2-22)
packages/plugin-vite/src/index.ts (2)
packages/runtime/src/client/ClientBuilder.ts (1)
build
(25-37)packages/runtime/src/client/Client.ts (1)
start
(40-43)
packages/runtime/src/client/Client.ts (1)
packages/runtime/src/ProcedureRunner.ts (1)
ProcedureRunner
(5-19)
packages/middleware/src/MiddlewareManager.ts (1)
packages/sourcing/src/index.ts (1)
ModuleImporter
(5-5)
packages/services/test/repository/fixtures/sourcingManager.fixture.ts (1)
packages/sourcing/src/LocalSourcingManager.ts (1)
LocalSourcingManager
(7-16)
🪛 Biome (1.9.4)
packages/sourcing/src/modules/RemoteModuleLocator.ts
[error] 8-12: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
packages/runtime/src/server/ServerBuilder.ts
[error] 7-7: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (60)
examples/resources/package.json (1)
2-2
: Approve example package renameThe package name has been correctly updated to
"jitar-resources-example"
to reflect the new example purpose.packages/sourcing/test/fixtures/TestFileSystem.ts (1)
4-4
: Update import path to reflect new file structureThe
LocalFileSystem
import has been updated to the newsrc/files
directory structure, aligning this test fixture with the refactored code organization in the sourcing package.packages/execution/package.json (1)
19-20
: Add dependency on@jitar/sourcing
The new dependency on
@jitar/sourcing
supports dynamic segment and resource loading in the execution package. Using"*"
for internal packages is consistent with the monorepo’s versioning strategy.packages/configuration/src/ConfigurationManager.ts (2)
3-3
: ImportLocalFileManager
instead of genericFileManager
Switched to
LocalFileManager
to leverage the specialized local file system implementation, matching the updated sourcing architecture.
21-21
: InstantiateLocalFileManager
with root pathReplaced the generic
FileManager
instantiation withLocalFileManager
, ensuring the correct subclass is used for all local file operations in configuration management.packages/build/src/BuildManager.ts (2)
4-4
: ImportLocalFileManager
from sourcingReplaced the previous
FileManager
import withLocalFileManager
to use the new specialized manager for local file handling within the build workflow.
23-26
: InstantiateLocalFileManager
for each project pathAll file manager instances (source, target, resources, segments) now use
LocalFileManager
, aligning with the sourcing package’s updated API and ensuring consistent local file handling.packages/execution/src/models/Application.ts (1)
15-18
: LGTM: Clean implementation of the clearSegments methodThis new method provides a clean way to remove all segments from the application, which is important for the enhanced lifecycle management introduced elsewhere in the codebase.
packages/sourcing/src/modules/interfaces/ModuleLocator.ts (1)
2-7
: Well-defined interface for module location resolutionClean, focused interface that establishes a clear contract for resolving module locations. This is a good abstraction to support both local and remote module loading strategies.
packages/cli/src/commands/StartServer.ts (1)
6-6
: Code simplification with LocalSourcingManagerGood refactoring to use the consolidated
LocalSourcingManager
instead of separate managers. This change aligns with the broader refactoring effort in the sourcing package and simplifies the server building process.Also applies to: 52-52
packages/sourcing/src/modules/interfaces/ModuleImporter.ts (1)
2-9
: Clean interface for asynchronous module importingWell-designed interface that establishes a clear contract for module importing functionality. The Promise return type correctly handles the asynchronous nature of module loading operations.
packages/sourcing/src/files/errors/RemoteFilesNotSupported.ts (1)
2-8
: Clean implementation of an error classThe error class is straightforward and follows best practices with a descriptive error message that clearly indicates the unsupported operation.
packages/sourcing/src/files/RemoteFileManager.ts (1)
2-10
: Implementation follows class hierarchy design patternThe RemoteFileManager correctly extends the FileManager base class and injects the RemoteFileSystem implementation. This follows good object-oriented design principles by using inheritance and dependency injection.
packages/sourcing/src/files/LocalFileManager.ts (1)
2-10
: Implementation follows class hierarchy design patternThe LocalFileManager correctly extends the FileManager base class and injects the LocalFileSystem implementation. This follows good object-oriented design principles by using inheritance and dependency injection.
packages/sourcing/src/modules/LocalImportManager.ts (1)
7-13
: Well-structured class with clear responsibility.The implementation follows good OOP principles by extending the base ImportManager with specific local functionality. The composition with LocalModuleLocator is appropriate for the separation of concerns between importing and locating modules.
packages/sourcing/src/files/FileManager.ts (2)
5-5
: Good implementation of the FileReader interface.Explicitly implementing the FileReader interface enforces a formal contract for file reading capabilities, which improves type safety and makes the API more clear.
Also applies to: 12-12
18-18
: Improved constructor with explicit dependency injection.Requiring a FileSystem parameter without defaults is a good practice that makes dependencies explicit and supports the separation of local and remote file management strategies.
packages/sourcing/src/modules/RemoteModuleLocator.ts (2)
7-12
: Security-focused implementation with clear documentation.The empty constructor with explanatory comments about security restrictions is a good practice. While static analysis suggests removing it as "unnecessary," keeping it maintains API consistency with other ModuleLocator implementations and provides a place for important security documentation.
🧰 Tools
🪛 Biome (1.9.4)
[error] 8-12: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
14-17
: Good security practice for remote module loading.Explicitly throwing an error with a clear message when attempting to load remote modules is an excellent security practice that prevents potential vulnerabilities.
packages/sourcing/src/RemoteSourcingManager.ts (1)
7-15
: Clean implementation with good separation of concerns.The RemoteSourcingManager properly extends SourcingManager and initializes the appropriate remote-specific components. The class composition follows good object-oriented design principles by delegating specialized behavior to RemoteFileManager and RemoteImportManager.
packages/plugin-vite/src/index.ts (1)
201-203
: Client startup sequence updated for lifecycle management compatibilityThe changes to explicitly create a client variable and call
client.start()
align with the broader lifecycle management improvements in the runtime components. This ensures proper initialization of the client according to the new lifecycle management approach.packages/sourcing/src/LocalSourcingManager.ts (1)
7-16
: Implementation looks goodThe
LocalSourcingManager
correctly extends theSourcingManager
base class and provides appropriate local implementations of file and import managers. This implementation follows good object-oriented design principles by composing the necessary dependencies and delegating to the parent class.packages/sourcing/src/modules/LocalModuleLocator.ts (1)
6-24
: Path handling logic looks goodThe
LocalModuleLocator
correctly implements theModuleLocator
interface and handles the conversion of absolute paths to relative paths before resolving them. The code is clear and the comment explains the purpose of the conversion.packages/sourcing/src/modules/ImportManager.ts (1)
1-33
: Well-designed module import abstraction with good error handling.The
ImportManager
class provides a clean abstraction for module importing with proper error handling and follows the dependency injection pattern by accepting aModuleLocator
. This separation of concerns will make testing and extending the import functionality easier.packages/sourcing/src/modules/index.ts (1)
1-11
: Clear and concise barrel file.This well-organized barrel file provides a clean interface for importing module-related components. The exports are organized by type (errors, interfaces, types, and implementation classes).
packages/sourcing/src/files/index.ts (1)
1-14
: Well-structured file exports.This barrel file provides a clean and organized way to export file-related components, making imports elsewhere in the codebase more concise and maintainable.
packages/runtime/src/server/ResourceManager.ts (1)
1-41
: Good implementation of resource lifecycle management.The
ResourceManager
class cleanly encapsulates the responsibility of running setup and teardown scripts, following the single responsibility principle. The use of private methods and fields makes the interface clean and implementation details hidden.packages/runtime/src/client/Client.ts (3)
40-43
: Improved lifecycle management with asynchronous operationsThe delegation to private async methods for start/stop operations provides a cleaner interface while allowing for proper orchestration of asynchronous initialization and cleanup.
Also applies to: 45-48
64-75
: Well-structured startup sequence with concurrent operationsThe implementation properly coordinates the startup sequence by:
- Starting both worker and middleware manager concurrently for better performance
- Adding the ProcedureRunner middleware only after both components are initialized
This approach ensures all dependencies are properly initialized before they're used.
77-84
: Proper shutdown sequence with concurrent operationsThe teardown method properly stops both the middleware manager and worker concurrently, providing efficient resource cleanup.
packages/sourcing/src/SourcingManager.ts (3)
3-6
: Good implementation of interfaces and improved import organizationThe SourcingManager now explicitly implements the FileReader and ModuleImporter interfaces, making its capabilities clear. The consolidated imports from './files' and './modules' improve code organization.
9-15
: Better dependency injection patternAdding the ImportManager parameter to the constructor follows the dependency injection pattern, making the class more modular and testable.
24-37
: Improved method implementations with clear delegationThe methods have been simplified by:
- Removing unnecessary
async
keywords where direct delegation is used- Delegating module importing entirely to the specialized ImportManager instance
This follows the single responsibility principle by letting specialized components handle their specific tasks.
packages/runtime/src/client/ClientBuilder.ts (3)
5-5
: Good import of new dependencyThe RemoteSourcingManager import is properly added to support the new sourcing capabilities.
27-34
: Well-structured build process with clear dependenciesThe build process now:
- Explicitly extracts configuration fields for clarity
- Creates a RemoteSourcingManager instance with the provided remoteUrl
- Passes the sourcing manager to both middleware and execution managers
This ensures proper integration of sourcing capabilities throughout the system.
39-55
: Updated method signatures with proper dependency injectionBoth private build methods now accept the sourcingManager as their first parameter and create their respective managers with it. This properly links sourcing capabilities to middleware and execution management.
packages/sourcing/src/files/RemoteFileSystem.ts (1)
5-13
: Clear placeholder implementation with good documentationThe class is well-documented as a placeholder for remote file systems, with clear comments explaining its current role and future directions.
packages/execution/src/ExecutionManager.ts (6)
2-3
: Clean module import for ModuleImporterThe addition of the ModuleImporter import from @jitar/sourcing aligns with the dynamic loading capabilities being added to this class.
27-29
: Appropriate private fields for module managementThese private readonly fields properly encapsulate the module importer and segment files, following TypeScript best practices.
34-38
: Well-structured constructorThe constructor properly initializes the module importer and segment files, with a sensible default empty array for segmentFiles.
40-48
: Clear lifecycle methods implementationThe start() and stop() methods provide a clean lifecycle management approach, delegating to private methods for actual implementation. This pattern is consistent with other managers in the codebase.
50-55
: Effective segment loading implementationThe loadSegment method correctly awaits the module import and adds the segment to the application. The casting of module.default to Segment is appropriate here.
116-124
: Well-implemented private helper methodsThe private methods effectively handle the loading and clearing of segments. The loadSegments method correctly uses Promise.all for parallel loading, while clearSegments delegates to the application's clearSegments method.
packages/middleware/src/MiddlewareManager.ts (6)
3-3
: Appropriate import for ModuleImporterThe addition of the ModuleImporter import from @jitar/sourcing aligns with the dynamic loading capabilities being added to this class.
11-13
: Well-defined private fieldsThese private readonly fields properly encapsulate the module importer and middleware files, following TypeScript best practices.
16-20
: Clear constructor implementationThe constructor properly initializes the module importer and middleware files, with a sensible default empty array for middlewareFiles.
22-30
: Consistent lifecycle methodsThe start() and stop() methods provide a clean lifecycle management approach, delegating to private methods for implementation. This follows the same pattern used in ExecutionManager.
32-37
: Effective middleware loading implementationThe loadMiddleware method correctly awaits the module import and adds the middleware to the application. The casting of module.default to Middleware is appropriate here.
68-71
: Well-implemented loadMiddlewares methodThis private method correctly uses Promise.all for parallel loading of all middleware files, which is an efficient approach.
packages/sourcing/src/index.ts (3)
2-3
: Clean export organization for file-related componentsThe reorganization of exports for file-related types and classes improves the module's structure and makes the API clearer.
5-6
: Well-structured module-related exportsThe clear separation of module-related types and exports improves code organization and makes dependencies more explicit.
9-10
: Good addition of sourcing managers exportsThe export of LocalSourcingManager and RemoteSourcingManager aligns with the refactoring goals and provides a clear API for consumers.
packages/services/src/worker/LocalWorker.ts (7)
20-20
: Good addition of registerAtGateway configuration optionThe optional registerAtGateway flag provides better control over the worker's behavior with respect to the gateway.
30-30
: Appropriate private field for registration flagThe private readonly field properly encapsulates the registration flag, following TypeScript best practices.
39-39
: Clear initialization of registerAtGatewayThe boolean conversion ensures the field is always a boolean value, avoiding potential undefined issues.
57-58
: Important lifecycle sequence improvementStarting the ExecutionManager before gateway operations ensures modules are loaded and ready before worker registration, which is a correct dependency ordering.
63-66
: Conditional gateway registrationThe conditional registration based on the registerAtGateway flag provides flexibility in controlling worker behavior, which can be useful in various deployment scenarios.
72-77
: Improved worker deregistration logicThe worker now only attempts to remove itself from the gateway if it has been assigned an ID, which prevents unnecessary calls.
81-82
: Proper execution manager shutdownThe ExecutionManager is now properly stopped after gateway operations, ensuring a clean shutdown sequence. The explicit await is important for proper async operation.
packages/health/src/HealthManager.ts (1)
36-41
: Missing validation of default export when loading a health check
loadHealthCheck()
assumes that the imported module exposes adefault
export that is a validHealthCheck
.
If a file is mis-configured (e.g.export const foo = …
) this will silently addundefined
, breaking the manager later.Consider guarding against this:
const module = await this.#moduleImporter.import(filename); const healthCheck = module.default; if (healthCheck === undefined) { throw new InvalidHealthCheck(); // or a dedicated error } this.addHealthCheck(healthCheck as HealthCheck);
async #runScripts(scripts: string[]): Promise<void> | ||
{ | ||
await Promise.all(scripts.map(script => this.#sourcingManager.import(script))); | ||
} |
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.
🛠️ Refactor suggestion
Add error handling for script execution failures.
The #runScripts
method currently has no error handling. If any script fails to import or execute, the Promise.all will reject with the first error encountered, potentially leaving the system in an inconsistent state. Consider adding try/catch blocks and a more graceful failure mechanism.
async #runScripts(scripts: string[]): Promise<void>
{
- await Promise.all(scripts.map(script => this.#sourcingManager.import(script)));
+ try {
+ await Promise.all(scripts.map(async script => {
+ try {
+ await this.#sourcingManager.import(script);
+ } catch (error) {
+ console.error(`Failed to run script ${script}:`, error);
+ // Consider whether to rethrow or continue with other scripts
+ throw error;
+ }
+ }));
+ } catch (error) {
+ console.error('Error running scripts:', error);
+ throw error; // Rethrow to notify caller of failure
+ }
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
async #runScripts(scripts: string[]): Promise<void> | |
{ | |
await Promise.all(scripts.map(script => this.#sourcingManager.import(script))); | |
} | |
async #runScripts(scripts: string[]): Promise<void> | |
{ | |
try { | |
await Promise.all(scripts.map(async script => { | |
try { | |
await this.#sourcingManager.import(script); | |
} catch (error) { | |
console.error(`Failed to run script ${script}:`, error); | |
// Consider whether to rethrow or continue with other scripts | |
throw error; | |
} | |
})); | |
} catch (error) { | |
console.error('Error running scripts:', error); | |
throw error; // Rethrow to notify caller of failure | |
} | |
} |
#clearMiddlewares(): void | ||
{ | ||
this.#middlewares.slice(); | ||
} |
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.
Bug in clearMiddlewares implementation
The current implementation doesn't actually clear the middlewares array. The slice() method returns a new array but doesn't modify the original.
Apply this fix:
#clearMiddlewares(): void
{
- this.#middlewares.slice();
+ this.clearMiddlewares();
}
Or alternatively:
#clearMiddlewares(): void
{
- this.#middlewares.slice();
+ this.#middlewares = [];
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
#clearMiddlewares(): void | |
{ | |
this.#middlewares.slice(); | |
} | |
#clearMiddlewares(): void | |
{ | |
- this.#middlewares.slice(); | |
+ this.#middlewares = []; | |
} |
packages/services/test/repository/fixtures/sourcingManager.fixture.ts
Outdated
Show resolved
Hide resolved
import { DummyProvider, DummyRunner, LocalGateway, LocalRepository, LocalWorker, Proxy, RemoteBuilder, RemoteGateway, RemoteRepository } from '@jitar/services'; | ||
import { SourcingManager } from '@jitar/sourcing'; |
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.
🛠️ Refactor suggestion
Avoid shadowing the built-in Proxy
global
Importing Proxy
without an alias makes every subsequent reference (e.g. new Proxy({ … })
) easy to confuse with the built-in Proxy
constructor that wraps objects in ECMAScript. Static-analysis is already warning about this.
-import { DummyProvider, DummyRunner, LocalGateway, LocalRepository, LocalWorker, Proxy, RemoteBuilder, RemoteGateway, RemoteRepository } from '@jitar/services';
+import {
+ DummyProvider,
+ DummyRunner,
+ LocalGateway,
+ LocalRepository,
+ LocalWorker,
+ Proxy as ServiceProxy,
+ RemoteBuilder,
+ RemoteGateway,
+ RemoteRepository
+} from '@jitar/services';
and update all occurrences of new Proxy(
→ new ServiceProxy(
within this file (≈7 places).
This tiny rename removes ambiguity and prevents accidental usage of the wrong Proxy
.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
import { DummyProvider, DummyRunner, LocalGateway, LocalRepository, LocalWorker, Proxy, RemoteBuilder, RemoteGateway, RemoteRepository } from '@jitar/services'; | |
import { SourcingManager } from '@jitar/sourcing'; | |
import { | |
DummyProvider, | |
DummyRunner, | |
LocalGateway, | |
LocalRepository, | |
LocalWorker, | |
Proxy as ServiceProxy, | |
RemoteBuilder, | |
RemoteGateway, | |
RemoteRepository | |
} from '@jitar/services'; | |
import { SourcingManager } from '@jitar/sourcing'; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
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.
After a short discussion we decided to rename the Proxy to LocalProxy to prevent confusion with the ECMAScript Proxy Object
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.
@basmasking Thanks for the update! That's a great decision to rename it to LocalProxy
– it clearly indicates its purpose while avoiding any confusion with JavaScript's built-in Proxy
object.
✏️ Learnings added
Learnt from: basmasking
PR: MaskingTechnology/jitar#638
File: packages/runtime/src/server/ServerBuilder.ts:7-8
Timestamp: 2025-05-13T18:15:31.378Z
Learning: In the Jitar codebase, always import the `Proxy` class from `@jitar/services` as `LocalProxy` to avoid shadowing the built-in JavaScript `Proxy` global object.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
#buildLocalWorker(url: string, configuration: WorkerConfiguration): LocalWorker | ||
{ | ||
const trustKey = configuration.trustKey; | ||
const gateway = configuration.gateway ? this.#buildRemoteGateway(configuration.gateway) : undefined; | ||
const executionManager = await this.#buildExecutionManager(configuration.segments); | ||
const registerAtGateway = gateway !== undefined; | ||
const executionManager = this.#buildExecutionManager(configuration.segments); | ||
|
||
return new LocalWorker({ url, trustKey, gateway, executionManager }); | ||
return new LocalWorker({ url, trustKey, gateway, registerAtGateway, executionManager }); | ||
} |
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.
Guard against segments
being undefined
WorkerConfiguration.segments
is forwarded straight into #buildExecutionManager
, which immediately calls .map
(line 161).
If the field is missing/optional the server will crash with TypeError: Cannot read properties of undefined (reading 'map')
.
-const executionManager = this.#buildExecutionManager(configuration.segments);
+const segmentNames = configuration.segments ?? [];
+const executionManager = this.#buildExecutionManager(segmentNames);
Equally, change the signature of #buildExecutionManager
to accept an optional array (see next comment).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
#buildLocalWorker(url: string, configuration: WorkerConfiguration): LocalWorker | |
{ | |
const trustKey = configuration.trustKey; | |
const gateway = configuration.gateway ? this.#buildRemoteGateway(configuration.gateway) : undefined; | |
const executionManager = await this.#buildExecutionManager(configuration.segments); | |
const registerAtGateway = gateway !== undefined; | |
const executionManager = this.#buildExecutionManager(configuration.segments); | |
return new LocalWorker({ url, trustKey, gateway, executionManager }); | |
return new LocalWorker({ url, trustKey, gateway, registerAtGateway, executionManager }); | |
} | |
#buildLocalWorker(url: string, configuration: WorkerConfiguration): LocalWorker | |
{ | |
const trustKey = configuration.trustKey; | |
const gateway = configuration.gateway ? this.#buildRemoteGateway(configuration.gateway) : undefined; | |
const registerAtGateway = gateway !== undefined; | |
const segmentNames = configuration.segments ?? []; | |
const executionManager = this.#buildExecutionManager(segmentNames); | |
return new LocalWorker({ url, trustKey, gateway, registerAtGateway, executionManager }); | |
} |
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.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/runtime/src/server/ServerBuilder.ts (1)
7-7
:⚠️ Potential issueRename
Proxy
import to avoid shadowing the built-in globalThe
Proxy
import shadows JavaScript's built-inProxy
class, which can cause confusion and potential bugs. This issue is also flagged by static analysis tools.-import { DummyProvider, DummyRunner, LocalGateway, LocalRepository, LocalWorker, Proxy, RemoteBuilder, RemoteGateway, RemoteRepository } from '@jitar/services'; +import { DummyProvider, DummyRunner, LocalGateway, LocalRepository, LocalWorker, Proxy as ServiceProxy, RemoteBuilder, RemoteGateway, RemoteRepository } from '@jitar/services';Then update all occurrences of
new Proxy
in the file tonew ServiceProxy
.🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🧹 Nitpick comments (5)
packages/sourcing/src/modules/RemoteImportManager.ts (1)
5-11
: Consider adding documentation about security implicationsThis class indirectly enforces security restrictions by using the
RemoteModuleLocator
, which throws an error for all locate attempts. Consider adding a class-level comment that explains this security behavior for better discoverability and maintainability.+/** + * Manages imports for remote modules. Currently prevents all remote module loading + * for security reasons by using RemoteModuleLocator which throws ModuleNotLoaded errors. + */ export default class RemoteImportManager extends ImportManager { constructor(location: string) { super(new RemoteModuleLocator(location)); } }packages/services/test/repository/fixtures/sourcingManager.fixture.ts (3)
11-14
: Consider documenting why filter throws NotImplemented.While throwing
NotImplemented
is appropriate for a test fixture that doesn't need this functionality, adding a brief comment explaining why it's intentionally not implemented would improve code clarity.
16-20
: Consider documenting why exists throws NotImplemented.Similar to
filter
, adding a brief comment explaining the design decision to not implement this method would help future developers understand the test fixture's intentions.
32-36
: Consider documenting why delete throws NotImplemented.As with the other unimplemented methods, adding a brief comment would improve code clarity.
packages/runtime/src/server/ServerBuilder.ts (1)
98-99
: Consider adding a documentation comment forregisterAtGateway
The new
registerAtGateway
flag is derived from the presence of a gateway, but its purpose and implications aren't immediately clear from the code.const gateway = configuration.gateway ? this.#buildRemoteGateway(configuration.gateway) : undefined; +// If gateway is defined, the worker should register itself with the gateway const registerAtGateway = gateway !== undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.json
is excluded by!**/package-lock.json
📒 Files selected for processing (7)
examples/README.md
(1 hunks)examples/resources/README.md
(1 hunks)packages/health/src/HealthManager.ts
(3 hunks)packages/middleware/src/MiddlewareManager.ts
(2 hunks)packages/runtime/src/server/ServerBuilder.ts
(6 hunks)packages/services/test/repository/fixtures/sourcingManager.fixture.ts
(2 hunks)packages/sourcing/src/modules/RemoteImportManager.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- examples/README.md
- examples/resources/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/health/src/HealthManager.ts
- packages/middleware/src/MiddlewareManager.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/sourcing/src/modules/RemoteImportManager.ts (2)
packages/sourcing/src/modules/ImportManager.ts (1)
ImportManager
(7-33)packages/sourcing/src/modules/RemoteModuleLocator.ts (1)
RemoteModuleLocator
(5-18)
packages/services/test/repository/fixtures/sourcingManager.fixture.ts (1)
packages/sourcing/src/LocalSourcingManager.ts (1)
LocalSourcingManager
(7-16)
🪛 Biome (1.9.4)
packages/runtime/src/server/ServerBuilder.ts
[error] 7-7: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (11)
packages/sourcing/src/modules/RemoteImportManager.ts (1)
1-11
: LGTM - Fixed naming inconsistencyThe implementation correctly addresses the previous naming inconsistency by correctly using
RemoteImportManager
as the class name, which properly aligns with:
- The file name
RemoteImportManager.ts
- The use of
RemoteModuleLocator
internally- The overall pattern of "Remote" prefix for remote operations
The class appropriately extends
ImportManager
and delegates toRemoteModuleLocator
for security handling.packages/services/test/repository/fixtures/sourcingManager.fixture.ts (4)
3-3
: Updated imports align with new architecture.The addition of
LocalSourcingManager
reflects the architectural changes in the sourcing system.
8-8
: Good refactoring to extend LocalSourcingManager.Changing from implementing
FileManager
to extendingLocalSourcingManager
is a significant improvement. This properly aligns with the new sourcing architecture and eliminates the need to reimplement common functionality.Most importantly, this change implicitly resolves the previous issue with the
import()
method by inheriting a working implementation fromLocalSourcingManager
instead of overriding it with one that throws.
22-30
: LGTM!The implementation of
read
correctly handles the test fixture cases and throws an appropriate error for unknown filenames.
39-39
: LGTM!Exporting a new instance with an empty string location is appropriate for a test fixture.
packages/runtime/src/server/ServerBuilder.ts (6)
27-28
: Clean restructuring of setup and teardown scriptsExtracting the setup and teardown scripts from the configuration and delegating their management to a dedicated
ResourceManager
is a good architectural decision, improving separation of concerns.Also applies to: 35-35
35-37
: Improved server composition with dedicated managersThe synchronous instantiation of resource, middleware, and health managers streamlines the server build process and aligns with the PR objectives of moving loading responsibilities into dedicated managers.
Also applies to: 41-41
58-58
: Successfully refactored service methods from async to syncConverting the gateway and worker builder methods from async to sync improves code clarity and performance by removing unnecessary asynchronous operations.
Also applies to: 66-66, 79-79, 94-102
137-143
: Well-structured ResourceManager implementationThe
#buildResourceManager
method properly transforms input filenames and creates a newResourceManager
instance with appropriate parameters, supporting the PR's objective of running setup scripts before loading modules.
145-157
: Clean implementation of manager builders with default parametersThe
#buildHealthManager
and#buildMiddlewareManager
methods properly handle default values and filename transformations, making them more robust against undefined inputs.
159-164
: Fixed potential undefined segments issueBy using a default empty array parameter, the code now correctly handles undefined segment configurations, addressing the previous review comment about guarding against
undefined
segments.
|
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.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/runtime/src/server/ServerBuilder.ts (1)
7-7
: Address static analysis warning about shadowing the globalProxy
The static analysis tool still flags the imported
Proxy
as shadowing the globalProxy
property. This was noted in a previous review comment and should be addressed for clarity and to avoid potential bugs. Rename the importedProxy
toServiceProxy
or similar.-import { DummyProvider, DummyRunner, LocalGateway, LocalRepository, LocalWorker, Proxy, RemoteBuilder, RemoteGateway, RemoteRepository } from '@jitar/services'; +import { DummyProvider, DummyRunner, LocalGateway, LocalRepository, LocalWorker, Proxy as ServiceProxy, RemoteBuilder, RemoteGateway, RemoteRepository } from '@jitar/services';Remember to update all occurrences of
Proxy
in the code to use the renamed import.🧰 Tools
🪛 Biome (1.9.4)
[error] 7-7: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🧹 Nitpick comments (1)
packages/sourcing/src/modules/RemoteModuleLocator.ts (1)
7-16
: Consider keeping the constructor despite static analysis warning.The static analysis tool flags the constructor as unnecessary, but I agree with your implementation choice. The constructor with its explanatory comments serves an important documentation purpose, making it clear that this is intentionally designed for future extensibility.
If you want to make the intent even clearer to static analyzers, you could add a
@ts-ignore
comment or configure your linter to ignore this specific instance.🧰 Tools
🪛 Biome (1.9.4)
[error] 11-16: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/runtime/src/server/ServerBuilder.ts
(6 hunks)packages/services/test/repository/fixtures/sourcingManager.fixture.ts
(2 hunks)packages/sourcing/src/files/LocalFileSystem.ts
(1 hunks)packages/sourcing/src/modules/ImportManager.ts
(1 hunks)packages/sourcing/src/modules/RemoteImportManager.ts
(1 hunks)packages/sourcing/src/modules/RemoteModuleLocator.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/sourcing/src/files/LocalFileSystem.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/sourcing/src/modules/RemoteImportManager.ts
- packages/sourcing/src/modules/ImportManager.ts
- packages/services/test/repository/fixtures/sourcingManager.fixture.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/runtime/src/server/ServerBuilder.ts
[error] 7-7: Do not shadow the global "Proxy" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
packages/sourcing/src/modules/RemoteModuleLocator.ts
[error] 11-16: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
🔇 Additional comments (7)
packages/sourcing/src/modules/RemoteModuleLocator.ts (2)
5-22
: Well-designed security boundary preventing remote module loading.This class effectively implements a security boundary by blocking remote module loading, which is a good practice for preventing security vulnerabilities. The clear error message and explanatory comments help future developers understand the design intent.
🧰 Tools
🪛 Biome (1.9.4)
[error] 11-16: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
18-21
: The error handling is clear and informative.The
locate
method appropriately throws aModuleNotLoaded
error with a descriptive message explaining why remote module loading is disallowed. This provides good feedback to developers who might attempt to use this functionality.packages/runtime/src/server/ServerBuilder.ts (5)
35-37
: Good refactoring to use synchronous manager building methodsThe refactoring to use synchronous manager building methods improves code readability and maintainability. This approach of directly instantiating manager classes aligns well with the PR objective of moving loading into dedicated managers.
94-102
: Good addition of theregisterAtGateway
flagAdding the
registerAtGateway
flag improves the clarity of worker-gateway interactions. This approach explicitly documents when a worker should register with a gateway based on the presence of a gateway configuration, making the code more maintainable and less error-prone.
137-143
: Well-designedResourceManager
integrationThe new
ResourceManager
implementation efficiently handles setup and teardown scripts. The translation of filenames to ensure proper extensions is a good practice. This change nicely centralizes resource management, which aligns with the PR objectives.
145-157
: Consistent pattern for manager instantiationThe implementation of
#buildHealthManager
and#buildMiddlewareManager
follows a consistent pattern, making the code more maintainable. The use of default empty arrays for parameters is a good defensive coding practice that prevents null/undefined errors.
159-164
: Good implementation of default parametersThe updated
#buildExecutionManager
now correctly handles the case when no segments are provided by using a default empty array. This addresses a previous comment and prevents potential runtime errors whensegments
is undefined.
{ | ||
filter(...patterns: string[]): Promise<string[]>; | ||
|
||
exists(filename: string): Promise<boolean>; |
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.
Nitpick: alphabetical order
import { DummyProvider, DummyRunner, LocalGateway, LocalRepository, LocalWorker, Proxy, RemoteBuilder, RemoteGateway, RemoteRepository } from '@jitar/services'; | ||
import { SourcingManager } from '@jitar/sourcing'; |
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.
After a short discussion we decided to rename the Proxy to LocalProxy to prevent confusion with the ECMAScript Proxy Object
Fixes #608
Changes proposed in this pull request:
@MaskingTechnology/jitar
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores