fix: collect imports from components in props in _get_all_imports()#6317
fix: collect imports from components in props in _get_all_imports()#6317lawrence3699 wants to merge 1 commit intoreflex-dev:mainfrom
Conversation
Fixes reflex-dev#6312 _get_all_imports() only walked self.children to collect transitive imports. Components embedded in props (e.g. a fallback: Component prop) were skipped, causing missing JS imports and ReferenceError at runtime. The sibling methods _get_all_custom_code() and _get_all_dynamic_imports() already call self._get_components_in_props() for this case. This brings _get_all_imports() in line with them.
Greptile SummaryThis PR fixes a bug where Confidence Score: 5/5Safe to merge — minimal, correct bug fix with a targeted regression test and no side effects. The change is a one-method fix that mirrors the exact pattern already used by two sibling methods in the same class. All type signatures are consistent, there is no new recursion risk beyond what already existed, and the regression test directly validates the corrected behavior. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["_get_all_imports(collapse=False)"] --> B["self._get_imports()"]
A --> C["for child in self.children\nchild._get_all_imports()"]
A --> D["✅ NEW: for component in\n_get_components_in_props()\ncomponent._get_all_imports()"]
D --> E["_get_component_prop_property\ncached_property"]
E --> F["iterate get_props()\nfilter BaseComponent | Var values\n_components_from(value)"]
A --> G["merge_parsed_imports(...)"]
G --> H{"collapse?"}
H -- "Yes" --> I["collapse_imports(imports_)"]
H -- "No" --> J["return imports_"]
style D fill:#90EE90,stroke:#228B22
Reviews (1): Last reviewed commit: "fix: collect imports from components in ..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
Fixes a bug in Reflex’s component import collection so that transitive imports are also gathered from components embedded in component-typed props (e.g., fallback=...), preventing missing JS imports and runtime ReferenceErrors during JSX execution.
Changes:
- Update
Component._get_all_imports()to traverse components found in props via_get_components_in_props(), in addition toself.children. - Add a regression unit test ensuring imports from a component passed through a component-typed prop are included.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
packages/reflex-base/src/reflex_base/components/component.py |
Extends _get_all_imports() traversal to include components referenced in props, aligning behavior with other tree-walking helpers. |
tests/units/components/test_component.py |
Adds a regression test covering imports discovered through a component-typed prop. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
All Submissions:
Type of change
Changes To Core Features:
closes #6312
_get_all_imports()only walkedself.childrento collect transitive imports. Components embedded in props (e.g. afallback: Componentprop) were skipped, causing missing JS imports andReferenceErrorat runtime.The sibling methods
_get_all_custom_code()(line 1608) and_get_all_dynamic_imports()(line 1649) already callself._get_components_in_props()for exactly this case._get_all_imports()was the only tree-walking method that didn't.Before: When a component like
rx.icon("crown")is only used inside a component-typed prop (and not elsewhere on the page), its library import (lucide-react) is not emitted in the generated JSX. The app crashes at runtime withReferenceError: LucideCrown is not defined.After: Imports from components in props are collected alongside children imports, matching the behavior of the sibling methods.
Tests: Added a regression test that creates a component with a
Component-typed prop and verifies imports from the inner component are included. The test fails without the fix and passes with it. All 125 existing component tests continue to pass.