Conversation
fb1e5d0 to
1a49abd
Compare
🚀 Package Preview Available!Install this PR's preview build with npm: npm i @base44-preview/cli@0.0.32-pr.249.2d6f02ePrefer not to change any import paths? Install using npm alias so your code still imports npm i "base44@npm:@base44-preview/cli@0.0.32-pr.249.2d6f02e"Or add it to your {
"dependencies": {
"base44": "npm:@base44-preview/cli@0.0.32-pr.249.2d6f02e"
}
}
Preview published to npm registry — try new features instantly! |
| "moduleResolution": "bundler", | ||
| "allowImportingTsExtensions": true, | ||
| "typeRoots": ["../node_modules/@types"], | ||
| "types": ["deno"] |
There was a problem hiding this comment.
Deno types are different from Node and in order to handle it without collision between types I need separate tsconfig
| import type { BackendFunction } from "@/core/resources/function/schema.js"; | ||
| import type { Logger } from "../createDevLogger"; | ||
|
|
||
| const __dirname = dirname(fileURLToPath(import.meta.url)); |
There was a problem hiding this comment.
This is the way to figure out where will be bundled deno runtime wrapper that I use for functions.
import.meta.url will be something like file:///Users/username/Development/test-project/cli/dist/cli/index.js
and I know that output of the wrapper will be next to cli folder
| clackLog.info( | ||
| `Loaded functions: ${functionManager.functionNames().join(", ")}`, | ||
| ); |
There was a problem hiding this comment.
here I'm using clack.log because server is not ready yet and dev command didn't send outro of clack
b23b5c7 to
1d65858
Compare
| @@ -1,5 +1,5 @@ | |||
| { | |||
| "$schema": "https://biomejs.dev/schemas/2.3.13/schema.json", | |||
There was a problem hiding this comment.
no related to PR, but now schema is aligned to our version
| } | ||
| }, | ||
| "files": { | ||
| "includes": ["**", "!**/dist/**", "!**/node_modules/**", "!**/tests/fixtures/**", "!**/*.d.ts"] |
There was a problem hiding this comment.
no related to PR, but the correct syntaxis is not to mention ** at the end
8c9635d to
c910d1d
Compare
|
Claude finished @artemdemo's task in 1m 35s —— View job PR Review Complete ✅I've reviewed PR #249 and the implementation looks solid overall. Here's my comprehensive feedback: ✅ Strengths
🔍 Issues & SuggestionsCritical Issues1. Security: spawn("deno", ["run", "--allow-all", WRAPPER_PATH], {This grants unrestricted permissions to user functions. Consider using granular permissions: spawn("deno", ["run", "--allow-net", "--allow-env", "--allow-read", WRAPPER_PATH], {Or document why full permissions are necessary for local dev. Fix this → 2. Missing Response Validation (routes/functions.ts:30) 3. Race Condition in // Register exit handler BEFORE setting timeout
runningFunc.process.on("exit", (code) => { ... });
const timeout = setTimeout(() => { ... }, READY_TIMEOUT);Code Quality Issues4. Console.log Usage (createDevLogger.ts:29-36)
However, I see the PR description mentions "server is not ready yet and dev command didn't send outro of clack" (review comment on main.ts:72). This is a valid exception for long-running dev server output, but should be documented. The 5. Incomplete Zod Validation
The proxy forwards user function responses without schema validation. While functions are user-controlled, consider validating at least the response structure (status code, headers) for early error detection during development. 6. Cross-platform Path Handling (function-manager.ts:14-16) const __dirname = dirname(fileURLToPath(import.meta.url));
const WRAPPER_PATH = join(__dirname, "../deno-runtime/main.js");This assumes the bundled structure. If the build output changes, this path breaks silently. Consider:
7. Missing Error Context in Proxy (routes/functions.ts:92-103) logger.error(`Function proxy error for "${functionName}":`, error);8. Unused const devLogger = createDevLogger(false);This suggests the prefixed variant isn't used. Consider removing it or documenting when each variant should be used. Minor Issues9. Biome Config Changes (biome.json) 10. Magic String in Readiness Detection (function-manager.ts:200) if (output.includes("Listening on")) {This string is duplicated between 11. Function Process Stderr Handling (function-manager.ts:159-164) 12. Missing Tests
No test files were added. Consider adding:
📋 Documentation13. Error Handling Docs Updated 14. Deno Runtime README
🎯 Recommendations
SummaryThis is a well-structured implementation that follows most project conventions. The core architecture is sound, and the code is generally high quality. The main concerns are:
Once these are addressed, this will be a solid addition to the CLI. Nice work on the clean separation of concerns and thoughtful error handling! 🎉 |
kfirstri
left a comment
There was a problem hiding this comment.
Amazing job 💪
I think we can continue talking about this tomorrow
src/cli/dev/dev-server/main.ts
Outdated
| const { port: userPort } = options; | ||
| const port = userPort ?? (await getPort({ port: DEFAULT_PORT })); | ||
|
|
||
| const { project } = await readProjectConfig(); |
There was a problem hiding this comment.
What do you think about injecting the project config object, and the function data as an argument to the createDevServer function? it will help us test this more easily..
- you can grab the project / function at the
dev.tscommand - pass it to the createDevServer
- less coupling for the dev-server 💪
sharing some thoughts: ideally we would have a coreSDK that would've been passed into createDevServer, and then when you'll do stuff like const project = coreSDK.project or const functions = coreSDK.functions.getAll() we would be able to mock the SDK in tests..
c2f12e1 to
fd91937
Compare
|
README check ran. 5 issue(s) found and applied: added missing commands (eject, connectors pull/push, types generate), updated deploy description to match codebase, and improved logout/whoami descriptions for consistency. README.md has been updated in this branch. |
Note
Description
This PR adds local development support for Base44 backend functions by introducing a Deno runtime integration. When running
base44 dev, functions are automatically started in Deno subprocesses and proxied through the dev server, allowing developers to test function deployments locally without deploying to production.Related Issue
None
Type of Change
Changes Made
deno-runtime/directory with a Deno-specific wrapper that patchesDeno.serveto inject dynamic portsFunctionManagerclass to spawn, manage, and proxy Deno function processes with port allocation and lifecycle management/api/apps/:appId/functions/:functionNameDependencyNotFoundErrorto handle missing Deno installation gracefully with helpful error messagescreateDevLoggerutility for consistent dev server logging with color-coded outputtsconfig.jsonfor Deno runtime to use@types/denoinstead of Node types, preventing type conflictsbiome.jsonto exclude entiredist/andnode_modules/directoriesTesting
npm test)Checklist
docs/(AGENTS.md) if I made architectural changesAdditional Notes
FUNCTION_PATH,FUNCTION_PORT,FUNCTION_NAME) to configure function executionFunctionManagerhandles process lifecycle, stdout/stderr piping, and graceful shutdown on SIGINT/SIGTERMdocs/error-handling.mdto document the newDEPENDENCY_NOT_FOUNDerror codedeno-runtime/explaining the separation between Node.js CLI and Deno function runtime🤖 Generated by Claude | 2026-02-18 16:45 UTC