fix: discover only initialized Dart functions#172
Conversation
There was a problem hiding this comment.
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.
|
|
||
| /// Top-level function declarations, used to follow helpers that are invoked | ||
| /// from the `runFunctions`/`fireUp` registration callback. | ||
| final Map<String, FunctionDeclaration> _topLevelFunctions = {}; |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Hey @kevmoo, thanks for the feedback and I agree. There are really two things bundled here:
- 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.
- 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.
Fixes Dart functions manifest discovery so the builder only includes functions registered during
runFunctions/fireUpinitialization.Fixes firebase/firebase-tools#10454