feat: add Dart runtime delegate for emulator#9966
feat: add Dart runtime delegate for emulator#9966Lyokone wants to merge 26 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"?
There was a problem hiding this comment.
I've added a constant 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?
There was a problem hiding this comment.
I've added enforcing for this Dart version
| { 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+)?
| dependencies: | ||
| # TODO(ehesp): Replace with published package version once available on pub.dev | ||
| firebase_functions: | ||
| path: ../ |
There was a problem hiding this comment.
Should we leave this as a relative path or update it?
| version: 1.0.0 | ||
|
|
||
| environment: | ||
| sdk: '>=3.0.0 <4.0.0' |
There was a problem hiding this comment.
Should we update the template constraint to match the runtime requirement of 3.8?
| } | ||
| } | ||
|
|
||
| async build(): Promise<void> { |
There was a problem hiding this comment.
Should a check be implemented in validate() or build() to run dart pub get if .dart_tool/package_config.json is missing or stale? I think without this, iiuc users would need to manually run pub get, before the deploy or the deployment will fail
natebosch
left a comment
natebosch
left a comment
There was a problem hiding this comment.
The Dart package template LGTM after some tweaks. I did not review any of the typescript. The extra checked in files should likely be dropped from this PR.
| (request) async { | ||
| return Response(200, body: 'Hello from Dart Functions!'); | ||
| }); |
There was a problem hiding this comment.
[optional]
| (request) async { | |
| return Response(200, body: 'Hello from Dart Functions!'); | |
| }); | |
| (request) async => Response(200, body: 'Hello from Dart Functions!'), | |
| ); |
| version: 1.0.0 | ||
|
|
||
| environment: | ||
| sdk: '>=3.0.0 <4.0.0' |
There was a problem hiding this comment.
| sdk: '>=3.0.0 <4.0.0' | |
| sdk: ^3.8.0 |
| name: functions | ||
| description: Firebase Functions for Dart |
There was a problem hiding this comment.
In the flutter and Dart templates we fill in a package name (I think based on the directory name). For this template assuming we need a fixed value I might expand it a little bit for clarity.
| name: functions | |
| description: Firebase Functions for Dart | |
| name: functions_template | |
| description: An app using Firebase Functions for Dart |
There was a problem hiding this comment.
Are these .dart_tool directory checkins intentional? They are not designed to be source controlled and I'm not sure of all the implications of doing that. The gitignore below correctly includes them.
There was a problem hiding this comment.
This file is also typically not checked in except for leaf dependency app packages.
|
|
||
| firebase.https.onRequest( | ||
| name: 'helloWorld', | ||
| options: const HttpsOptions(cors: Cors(['*'])), |
There was a problem hiding this comment.
can we also set maxInstances? maybe globally like JS if that works in Dart?
firebase-tools/templates/init/functions/javascript/index.js
Lines 14 to 24 in 87ec0f7
5 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