feat: add Dart runtime delegate for emulator#9966
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.
Lyokone
force-pushed
the
@invertase/dart
branch
from
71c0b7f to
372c7a5
Compare
Lyokone
force-pushed
the
@invertase/dart
branch
from
1881a9b to
7fb8b34
Compare
| 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?
There was a problem hiding this comment.
As soon as it's published we'll be able to add the correct version
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.
inlined
left a comment
inlined
left a comment
There was a problem hiding this comment.
Only feedback I'd ask you to critically think about is whether the dart behavior of having a shared pool should be all run functions.
With CRF I was thinking that we could group 1..n functions in a service. So one codebase of N functions can deploy 1 <= M <= N services. You split basically if you have security or resource reasons to split (at least for BG functions. For callable/https functions the URL changes and we need to spec that out).
Aside: Is that how Dart is going to work in prod too?
| { exit: 1 }, | ||
| ); | ||
| } | ||
| return Promise.resolve(new Delegate(context.projectId, context.sourceDir, runtime)); |
There was a problem hiding this comment.
You don't need to call Promise.resolve if you're in an async function
| const match = /Dart SDK version:\s*(\d+\.\d+\.\d+)/.exec(versionOutput); | ||
| if (match) { | ||
| const installedVersion = match[1]; | ||
| if (installedVersion.localeCompare(MIN_DART_SDK_VERSION, undefined, { numeric: true }) < 0) { |
There was a problem hiding this comment.
Fascinating method of doing this. Do you want to make sure you blindly accept all versions of dart instead of using the semver library and doing semver arithmetic?
|
|
||
| // Verify pubspec.yaml exists and is readable. | ||
| const pubspecYamlPath = path.join(this.sourceDir, "pubspec.yaml"); | ||
| try { |
There was a problem hiding this comment.
This is the more modern version of what you do in tryCreateDelegate. Why not reconcile to just one?
|
|
||
| // Verify the entry point file exists. | ||
| const entryPointPath = path.join(this.sourceDir, this.entryPoint); | ||
| try { |
There was a problem hiding this comment.
If you're going to check for three files using async libraries you might as well do it in parallel
|
|
||
| // Run `dart pub get` if dependencies have not been resolved yet. | ||
| const packageConfigPath = path.join(this.sourceDir, ".dart_tool", "package_config.json"); | ||
| try { |
There was a problem hiding this comment.
Always prefer to return early and avoid nesting. If this try is effectively an if statement, consider something like:
try {
await access
return;
} catch () { /* noop */ }
// Install dependencies
Also, all of this is probably because exists is deprecated and you don't have a promise version? The reasons for its deprecation are basically irrelevant here (it's encouraged for you to try to do the thing rather than using exists as a dry run).
I might just define exists using promisify and simplify all this code:
const [pubspecExists, entryPointExists, packageConfigExists] = await Promise.all([pubspecYamlPath, entryPointPath, packageConfigPath]).map(exists);
if (!pubspecExists) {
throw error
}
if (!entryPointExists) {
throw erroR
}
if (packageConfigExists) {
return;
}
// install pacakges
| await fs.promises.access(packageConfigPath, fs.constants.R_OK); | ||
| } catch { | ||
| logLabeledBullet("functions", "running dart pub get..."); | ||
| const pubGetProcess = spawn(this.bin, ["pub", "get"], { |
There was a problem hiding this comment.
I know this is copying other code but I wonder if there's an opportunity for a utility here that we can use throughout the codebase...
import { spawn } from 'node:child_process';
import { createInterface } from 'node:readline';
import { once } from 'node:events';
async function execChild({
bin: string,
args: string[],
cwd?: string,
logsPrefix?: string,
): Promise<number> {
const child = spawn('dart', ['pub', 'get'], { stdio: ['ignore', 'pipe', 'pipe'] });
child.stderr?.pipe(child.stdout!);
const rl = createInterface({ input: child.stdout!, terminal: false });
const printOutput = (async () => {
for await (const line of rl) {
logger.debug(logsPrefix ? `${logsPrefix} ${line.trim}` ? line.trim()`);
}
})();
const processEnd = Promise.race([
once(child, 'close'),
once(child, 'error').then(([err]) => { throw err; })
]);
const [_, exitCode] = await Promise.all([printOutput, processEnd]);
return exitCode;
}
Then you could just say
const exitCode = await execChild({ bin: this.bin, args: ["pub", "get"], logsPrefix: "[dart pub get"]});
if (exitCode) {
throw new FirebaseError(...);
}
here and below say
const buildRunnerExit = await execChild({
bin: this.bin,
args: ["run", "build_runner", "build", "--delete-conflicting-outputs"],
cwd: this.sourceDir,
loggerPrefix: "[build_runner]"
});
// ...
const compileExit = await execChild({
bin: this.bin,
args: ["compile", "exe", this.entryPoint, "-o", "bin/server", "--target-os=linux", "--target-arch=x64"],
cwd: this.sourceDir,
logsPrefix: "[dart compile]",
});
| * Returns a cleanup function that stops the build_runner process. | ||
| * The returned promise resolves once the initial build completes. | ||
| */ | ||
| async watch(onRebuild?: () => void): Promise<() => Promise<void>> { |
|
|
||
| // For Dart, include the function name in the path so the server can route | ||
| // For other runtimes, use / as they use FUNCTION_TARGET env var | ||
| const isDart = isLanguageRuntime(record.backend.runtime, "dart"); |
There was a problem hiding this comment.
I know this is a bit of a bigger change, but.... what if we just make this the behavior when platform is "run" and always make dart emit "run"?
We'll almost certainly want to investigate allowing collocation of functions in Run services if/when we move other languages to CRF.
| runtime = await this.startPython(backend, { ...runtimeEnv, ...secretEnvs }); | ||
| } else if (isLanguageRuntime(backend.runtime, "dart")) { | ||
| runtime = await this.startDart(backend, { ...runtimeEnv, ...secretEnvs }); | ||
| } else { |
There was a problem hiding this comment.
I think we now have the ability to now say
} else if (isLangaugeRuntime(backend.runtime, "nodejs")) {
// ...
}
assertExhaustive(backend.runtime);
* feat: Add dart delegate * feat: add support for hot reloading * fix: wait for build_runner initial build to complete * fix rooting * emulator update * update functions.yaml directory * clean run on emulator * clear project * improve runtime checks * merge fixes * clean * Fixing feedback from Gemini * compile before deploy * remove placeholder runtime * cross compiling * emulator fix * fix rooting * fix emulator detection * skipping compilation in emulator run * clean output * prevent duplicate watches * dart entry point and min dart version enforcing * change entry point to bin/server.dart * update template --------- Co-authored-by: Elliot Hesp <elliot.hesp@gmail.com> Co-authored-by: Darren Ackers <ackers86@hotmail.com>
9 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