Skip to content

fix: discover only initialized Dart functions#172

Open
Lyokone wants to merge 4 commits into
mainfrom
feat/dart-builder-registration-discovery
Open

fix: discover only initialized Dart functions#172
Lyokone wants to merge 4 commits into
mainfrom
feat/dart-builder-registration-discovery

Conversation

@Lyokone

@Lyokone Lyokone commented May 6, 2026

Copy link
Copy Markdown
Contributor

Fixes Dart functions manifest discovery so the builder only includes functions registered during runFunctions/fireUp initialization.
Fixes firebase/firebase-tools#10454

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request enhances the Firebase Functions manifest builder to accurately discover function registrations within runFunctions or fireUp callbacks, including those invoked via top-level helper functions. It introduces a mechanism to track registration depth and traverse helper function bodies during the collection phase. A review comment suggested refining the detection of registration runners by ensuring they are top-level calls (where target == null) to prevent accidental matches with object methods of the same name.

Comment thread lib/builder.dart
Comment thread lib/builder.dart Outdated

/// Top-level function declarations, used to follow helpers that are invoked
/// from the `runFunctions`/`fireUp` registration callback.
final Map<String, FunctionDeclaration> _topLevelFunctions = {};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we allow registrations from functions that are NOT main, as long as they are called from main?

The complexity here makes me nervous.

It's good to fix this issue, but I'm not aware of how complicated this is!

@Lyokone Lyokone May 29, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @kevmoo, thanks for the feedback and I agree. There are really two things bundled here:

  1. The fix for #10454, today the builder deploys every onX() call in the source, even unreachable ones. Gating discovery to the runFunctions/fireUp callback is what we actually need.
  2. The helper-following, it only handles same-file top-level helpers; it can't follow cross-file helpers, methods, closures, or loops, all of which the runtime registers fine. So it looks more capable than it is.

I'll drop the helper-following and keep just the gating fix. The contract becomes: register functions directly inside the callback. I'll document that and add a test. If we want multi-file organization later, we should design it explicitly rather than infer it from the AST.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improper dart functions initialization in emulator

2 participants