feat: add Dart runtime delegate for emulator#9966
feat: add Dart runtime delegate for emulator#9966Lyokone wants to merge 23 commits intofirebase:mainfrom
Conversation
Summary of ChangesHello @Lyokone, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands Firebase Functions capabilities by introducing experimental support for Dart as a runtime. It provides a comprehensive Dart runtime delegate, integrates Dart's Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request introduces Dart runtime support for Firebase Functions, including a new runtime delegate, emulator support with hot reload via build_runner watch, and scaffolding for new Dart projects during firebase init functions. The changes involve adding new files for Dart-specific configurations and modifying existing TypeScript files to integrate Dart into the functions emulator and runtime detection logic. The code appears to be well-structured and follows existing patterns for other runtimes like Python and Node.js. The changes also include updates to the .gitignore template and the firebase.json configuration to properly handle Dart projects.
src/init/features/functions/dart.ts
Outdated
| import { latest } from "../../../deploy/functions/runtimes/supported"; | ||
|
|
||
| // TODO(ehesp): Create these template files in templates/init/functions/dart/ | ||
| // TODO(ehesp): Dont use relative path for firebase_functions |
There was a problem hiding this comment.
The TODO comment regarding firebase_functions relative path should be addressed. Using a relative path for a dependency can lead to issues in different environments or when the project structure changes. It's generally better to use a published package or a more robust dependency management approach.
// TODO(ehesp): Create these template files in templates/init/functions/dart/
// TODO(ehesp): Use a published package for firebase_functions or a more robust dependency management approach.
src/init/features/functions/dart.ts
Outdated
|
|
||
| dependencies: | ||
| firebase_functions: | ||
| path: ../ |
There was a problem hiding this comment.
Using a relative path for firebase_functions in pubspec.yaml can be problematic for deployment and dependency resolution. It's generally recommended to use a published package from pub.dev or a git dependency with a specific ref for better stability and maintainability.
firebase_functions: ^x.y.z # Replace with the actual version from pub.dev
# path: ../ # Remove or comment out this line if using a published package|
|
||
| // Necessary for the GCF API to determine what code to load with the Functions Framework. | ||
| // Will become optional once "run" is supported as a platform | ||
| entryPoint: string; |
There was a problem hiding this comment.
This comment is no longer accurate as entryPoint is now required for all platforms, including run (which is normalized to gcfv2 for emulator compatibility). It can be removed.
| if (code !== 0 && code !== null) { | ||
| logger.debug(`build_runner exited with code ${code}. Hot reload may not work.`); | ||
| if (!initialBuildComplete) { | ||
| rejectInitialBuild(new Error(`build_runner exited with code ${code}`)); | ||
| } |
There was a problem hiding this comment.
The error message for build_runner exiting with a non-zero code should be more informative. It currently suggests that hot reload 'may not work', but if the initial build fails, hot reload definitely won't work, and the user should be informed that the functions might not be deployed or emulated correctly.
logger.debug(`build_runner exited with code ${code}. Initial build failed.`);
if (!initialBuildComplete) {
rejectInitialBuild(new FirebaseError(`build_runner exited with code ${code}. Your Dart functions may not be deployed or emulated correctly.`));
}| logger.debug(`Failed to start build_runner: ${err.message}`); | ||
| if (!initialBuildComplete) { | ||
| rejectInitialBuild(err); | ||
| } |
There was a problem hiding this comment.
The error message for build_runner failing to start should be more specific about the impact on the functions, similar to the previous comment. If build_runner cannot start, the functions will not be built or emulated.
logger.debug(`Failed to start build_runner: ${err.message}. Your Dart functions may not be deployed or emulated correctly.`);
if (!initialBuildComplete) {
rejectInitialBuild(err);
}
src/emulator/functionsEmulator.ts
Outdated
| const record = this.getTriggerRecordByKey(this.getTriggerKey(trigger)); | ||
| const pool = this.workerPools[record.backend.codebase]; | ||
| if (!pool.readyForWork(trigger.id)) { | ||
| const runtime = record.backend.runtime; |
There was a problem hiding this comment.
The runtime variable is declared but not used in this block. It should be removed to avoid unnecessary declarations.
if (!pool.readyForWork(trigger.id, record.backend.runtime)) {
src/emulator/functionsEmulator.ts
Outdated
| } | ||
| } | ||
| const worker = pool.getIdleWorker(trigger.id)!; | ||
| const worker = pool.getIdleWorker(trigger.id, runtime)!; |
There was a problem hiding this comment.
The runtime variable is declared but not used in this block. It should be removed to avoid unnecessary declarations.
const worker = pool.getIdleWorker(trigger.id, record.backend.runtime)!;
src/emulator/functionsEmulator.ts
Outdated
|
|
||
| const pool = this.workerPools[record.backend.codebase]; | ||
| if (!pool.readyForWork(trigger.id)) { | ||
| const runtime = record.backend.runtime; |
There was a problem hiding this comment.
The runtime variable is declared but not used in this block. It should be removed to avoid unnecessary declarations.
if (!pool.readyForWork(trigger.id, record.backend.runtime)) {| if (isLanguageRuntime(runtime, "dart")) { | ||
| return "~dart-shared~"; | ||
| } |
There was a problem hiding this comment.
The comment states that Dart loads all functions and routes based on request path, which is true. However, the current implementation uses ~dart-shared~ as the key, which means all Dart functions in a codebase will share the same worker. This is consistent with the Python runtime, but the comment could be more explicit about this shared process model for Dart.
// For Dart, use a shared key so all functions in a codebase share the same process.
// Dart loads all functions and routes based on request path, similar to Python.
if (isLanguageRuntime(runtime, "dart")) {
Lyokone
force-pushed
the
@invertase/dart
branch
from
71c0b7f to
372c7a5
Compare
Lyokone
force-pushed
the
@invertase/dart
branch
from
1881a9b to
7fb8b34
Compare
| [ | ||
| "compile", | ||
| "exe", | ||
| "lib/main.dart", |
There was a problem hiding this comment.
Are we sure we want to keep this entry point hard coded like this for "lib/main.dart"?
| return; | ||
| } | ||
|
|
||
| // Requires Dart 3.8+ for --target-os and --target-arch support. |
There was a problem hiding this comment.
Do you think it's worthwhile to enforce this "Requires Dart 3.8+" or check to provide a error message if the user's local Dart SDK is too old?
| { exit: 1 }, | ||
| ); | ||
| } | ||
| return Promise.resolve(new Delegate(context.projectId, context.sourceDir, runtime)); |
There was a problem hiding this comment.
Do you think it makes sense to add a check like in validate() that explicitly verifies the binary exists and optionally checks the version (going back to the other comment I left about requiring Dart 3.8+)?
3 participants
Adds Dart as a new runtime for Firebase Functions, gated behind the functionsrunapionly experiment flag.
build_runner, and manages build_runner watch for hot reload via the delegate's watch() method.
instead of FUNCTION_TARGET env var.
Scenarios Tested
Sample Commands