Add compiler plugin hooks and plugins and move compilation pipeline out of App#6260
Conversation
Greptile SummaryThis PR replaces Reflex's frontend compilation pipeline with a plugin-driven architecture: a Confidence Score: 5/5Safe to merge; no P0/P1 findings, only minor style and edge-case suggestions. All identified issues are P2. The core pipeline logic is well-structured, the sequential execution model eliminates the concurrency concerns of the old executor abstraction, and the test suite is extensive (2000+ new test lines). The deprecated App.theme discard and the re-walk double-apply are known/documented trade-offs. packages/reflex-base/src/reflex_base/plugins/compiler.py (_compile_component_with_replacements re-walk behavior) and packages/reflex-components-radix/src/reflex_components_radix/plugin.py (apply_app_theme silent discard) Important Files Changed
Sequence DiagramsequenceDiagram
participant App
participant compile_app
participant CompileContext
participant CompilerHooks
participant DefaultPagePlugin
participant ApplyStylePlugin
participant DefaultCollectorPlugin
participant MemoizeStatefulPlugin
participant RadixThemesPlugin
App->>compile_app: _compile()
compile_app->>compile_app: _resolve_radix_themes_plugin()
compile_app->>CompileContext: create(hooks=CompilerHooks(plugins))
compile_app->>CompileContext: compile(evaluate_progress, render_progress)
loop For each page
CompileContext->>CompilerHooks: eval_page(page_fn, page)
CompilerHooks->>DefaultPagePlugin: eval_page() → PageContext
DefaultPagePlugin-->>CompileContext: PageContext
CompileContext->>CompilerHooks: compile_component(root, page_ctx)
CompilerHooks->>RadixThemesPlugin: enter_component (auto-enable)
CompilerHooks->>ApplyStylePlugin: enter_component (apply style)
CompilerHooks->>MemoizeStatefulPlugin: enter_component (wrap stateful)
CompilerHooks->>DefaultCollectorPlugin: leave_component (collect imports/hooks)
CompileContext->>CompilerHooks: compile_page(page_ctx)
CompilerHooks->>DefaultCollectorPlugin: compile_page (collapse imports)
CompilerHooks->>RadixThemesPlugin: compile_page (inject theme wrap)
CompileContext->>compile_app: page_ctx.output_path, output_code
end
compile_app->>compile_app: _resolve_app_wrap_components()
compile_app->>compile_app: compile_memo_components()
compile_app->>compile_app: compile_root_stylesheet(plugins)
compile_app->>compile_app: write all outputs to disk
Reviews (3): Last reviewed commit: "Address CR feedback" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
masenf
left a comment
masenf
left a comment
There was a problem hiding this comment.
we need a test case for the child replacement logic when traversing the tree; we might not strictly need it now, but it will be important when moving the StatefulComponent compilation into the plugin system.
|
Khaleel and I were discussing this a bit further, and I think it would be better to add these new plugin hooks and hook dispatching system to the existing Plugin base in |
Move the frontend compilation pipeline from App._compile into compiler.compile_app(), introducing a CompilerPlugin protocol with enter_component/leave_component/eval_page/compile_page hooks. Remove the ExecutorType/ExecutorSafeFunctions abstractions in favor of a sequential plugin-driven compilation model.
FarhanAliRaza
force-pushed
the
compiler-hooks
branch
from
ac4af22 to
15f363b
Compare
FarhanAliRaza
changed the title
Move memo component compilation after app_root resolution so app-wrap components are included. Fix DefaultPagePlugin to preserve Var-backed titles instead of replacing them with the default string.
Move _get_app_wrap_components collection outside the `if stateful_component is None` guard so that app wrap components (e.g. UploadFilesProvider) are collected even when a component is wrapped as a stateful component. Add test verifying upload pages correctly emit UploadFilesProvider in the app root.
|
@masenf i dont know why the tests are failing. |
Remove the class-level tag_to_stateful_component dict and instead thread a compile-scoped stateful_component_cache through compile_from() and create(). This prevents stale cache entries from leaking between independent compilation runs. Also collect imports and app_wrap_components from root components in CompileContext so stateful component libraries and providers (e.g. UploadFilesProvider) are properly propagated. Update benchmarks to inline helpers using the new plugin API and add tests covering shared stateful components across pages and cache isolation between runs.
Component._get_vars had a dead-code cache path: `getattr(self, "__vars", None)` reads the literal attribute `__vars` but `self.__vars = []` writes to the name-mangled `_Component__vars`. The cache branch was never taken, and even if the name-mangling were fixed the missing `return` after `yield from vars` would have caused duplicate yields on repeated calls. Fix the cache (as `_vars_cache`) with a proper early-return. Extend the same per-instance cache pattern to `_get_imports` and `_get_hooks_internal`, which share the same dependency on `event_triggers` / `_get_vars`. Unify invalidation with the existing render-cache clear in `StatefulComponent.create` so all four caches drop together when `_fix_event_triggers` mutates the component.
| Returns: | ||
| A page context when the plugin can evaluate the page, otherwise ``None``. | ||
| """ | ||
| del page_fn, kwargs |
There was a problem hiding this comment.
why does it del the local vars?
There was a problem hiding this comment.
They are gonna be overloaded by the actual plugin. So Claude is i think, just trying to use it. Deleting it.
| ] | ||
|
|
||
|
|
||
| class CompilerPlugin(Protocol): |
There was a problem hiding this comment.
is this protocol needed since we have the base plugin definition?
|
|
||
|
|
||
| @dataclasses.dataclass(kw_only=True) | ||
| class BaseContext: |
There was a problem hiding this comment.
this class also gets defined in my backend event loop PR as reflex_base.context.base depending on whose merges first, we can refactor
| if isinstance(page_ctx.root_component, StatefulComponent): | ||
| self.all_imports = merge_imports( | ||
| self.all_imports, | ||
| page_ctx.root_component._get_all_imports(), |
There was a problem hiding this comment.
is this still making the recursive call? isn't this information cached in the PageContext as imports?
…ields Remove the duplicated _compile_component_without_replacements and _compile_component_single_enter_fast_path methods in favor of a single _compile_component_tree walker with inline replacement checks. This eliminates ~200 lines of duplication and removes several optimization-only fields (_regular_leave_component_hook_binders, _stateful_leave_component_hook_binders, _component_hooks_can_replace, _enter_component_hooks, _leave_component_hooks) with negligible benchmark impact.
_compile_stateful_components now returns its collected imports alongside the rendered code. CompileContext merges these into all_imports after compilation, and no longer calls _get_all_imports on the root component (the single-pass walk already collects structural imports).
CompilerPlugin duplicated methods already on Plugin base class. PageDefinition was a structural typing protocol only satisfied by UnevaluatedPage. Replace both with their concrete counterparts. Give UnevaluatedPage fields defaults so it can be used directly in tests and benchmarks, eliminating FakePage and BenchmarkPage classes.
# Conflicts: # packages/reflex-base/src/reflex_base/components/component.py # reflex/app.py
masenf
left a comment
masenf
left a comment
There was a problem hiding this comment.
i noticed another problem here with Foreach and Cond, and I think the fix is similar to what i did for Bare: basically treat these components as MemoizationLeaf and extract them to their own memoized component without the {children} hole.
When we're compiling out a Foreach, Cond, or Match, these need to be extracted as their own auto-memo component if they are stateful. And to ensure that nested Foreach is working, we need to treat the first one we encounter in the tree as a MemoizationLeaf so the nested Foreach can access the outer Foreach loop variables.
Ideally we have some way of separately memoizing these and passing the loop props down if needed, but that's a lot of extra complexity, so we should just get the base case working in the same way the current compiler does it.
The main difference between the approach here and the current compiler is that the current compiler would treat the parent component as stateful if any of its children are Foreach/Cond/Match, but that's not really appropriate any more with the {children} passthrough being the default, so these really should be memoized as Fragment containing their contents.
| # template. | ||
| snapshot_only = is_snapshot_boundary(component) | ||
|
|
||
| captured_hole_child: list[Component] = [] |
There was a problem hiding this comment.
why is this a list[Component] and not Component | None? The latter seems more semantically accurate to how it's used.
Consolidate auto-memo render strategy classification so snapshot bodies are used for MemoizationLeaf components and structural Foreach/Cond/Match forms, while passthrough components like forms keep their children hole for ref collection. Add regression tests proving special-form state hooks render inside generated memo components instead of leaking into the page, and update the automemo_upload example to validate stateful, upload, form, and special-form memoization output.
When Cond or Match cases contain stateful children, auto-memoize these separately from the component to avoid unnecessary mixing of hooks.
The previous implementation rebuilt nested `str([...])` representations at every recursion level and md5-hashed each leaf, dominating the auto- memo tagging path. Replacing it with one type-tagged, length-prefixed walker over a single hasher drops hashing samples ~3x in flexgen compile (~8% wall on top of the prior merge_imports fix).
Add functional test cases for Match and Cond in an integration test
Avoid special cases for these components. Instead, map the special children prop to the conditional cases at render time. Even if the children prop is a frontend placeholder from an experimental memo component, it will still be indexed and render properly.
|
With the latest commits, we are very close. I'm noticing a few remaining problems that shouldn't be too hard to fix: MemoizationLeaf components should still be memoized if anything in their subtree is stateful. i.e. AccordionTrigger is not getting memoized, even though the trigger contains a cond that depends on a state variable. I think the title and meta components (and potentially other html components) should be marked as MemoizationLeaf so we don't end up memoizing a stateful Bare that is passed to them. These elements cannot have children, so they don't render properly when we do this for example: jsx("title", {}, jsx(Bare_comp_cbb10fd3740c4ca79b576489b0269aed, {})),We should get two test cases and fixes for these. I'm signing off for now, but I might push commits in a few hours if I don't see anything here. |
Copy the original component's `_get_all_refs` onto the memo wrapper so that handleSubmit can be properly generated, even when the form itself is moved to its own passthrough-memoized component that doesn't actually know its children.
Snapshot boundaries (recursive=False) suppress descendants but were not themselves wrapped when their state lived in a child, so the state read leaked into the page module instead of the boundary's snapshot body. _should_memoize now walks descendants and wraps the boundary when any carry state-bearing or hooks-bearing Var data, event_triggers, or explicit user hooks. Refs from a static id prop are intentionally ignored so static-id-only elements stay unwrapped. The walk uses an id() seen-set so var_data.components reachable via multiple Vars (e.g. Foreach renderers) is visited once.
Mark void/raw-text HTML elements (title, meta, style, textarea, script, br, hr, img, input, link, base, area, col, embed, source, track, wbr, plus SVG desc/title/script/style and base.RawLink/ScriptTag) as recursive=False so a stateful Bare child stays an inline text interpolation rather than rendering as jsx(El, {}, jsx(Bare_xxx, {})), which is invalid for void elements and stringifies to [object Object] for raw-text ones. Shared constant lives in constants.compiler.
Make Component._get_vars(include_children=True) yield per child so the new subtree scan can short-circuit on first hit instead of materialising the whole subtree.
masenf
left a comment
masenf
left a comment
There was a problem hiding this comment.
in test code, imports should be at the top of the module. the only exception to this is integration tests which need imports in the app function because that ends up getting written out to its own module by the AppHarness
|
@greptile-apps re-review |
To pass `_should_memoize`, the component/var must contain state or hooks
|
ugh just realized that dynamic components are broken with this change import reflex as rx
class State(rx.State):
count: int = 0
@rx.event
def set_count(self, count: int):
self.count = count
@rx.var
def counter_ui(self) -> rx.Component:
return rx.hstack(
rx.button("-", on_click=State.set_count(self.count - 1)),
rx.text(self.count, size="9"),
rx.button("+", on_click=State.set_count(self.count + 1)),
spacing="5",
justify="center",
)
def index() -> rx.Component:
return rx.container(
rx.color_mode.button(position="top-right"),
rx.vstack(
rx.heading("Welcome to Reflex!", size="9"),
State.counter_ui,
),
)
app = rx.App()
app.add_page(index)This counter actually works on 0.9.1, but doesn't render the event handlers with this PR. Generated code on this PR: //__reflex_evaluate
const React = window.__reflex.react;
const { jsx } = window.__reflex["@emotion/react"];
const { Fragment, useEffect } = window.__reflex["react"];
const {
Button: RadixThemesButton,
Flex: RadixThemesFlex,
Text: RadixThemesText,
} = window.__reflex["@radix-ui/themes"];
export default function MySSRComponent() {
return jsx(
RadixThemesFlex,
{
align: "start",
className: "rx-Stack",
direction: "row",
justify: "center",
gap: "5",
},
jsx(RadixThemesButton, {}, "-"),
jsx(RadixThemesText, { as: "p", size: "9" }, -1),
jsx(RadixThemesButton, {}, "+"),
);
}Generated code on 0.9.1 //__reflex_evaluate
const React = window.__reflex.react;
const { jsx } = window.__reflex["@emotion/react"];
const { Fragment, useContext, useEffect } = window.__reflex["react"];
const {
Button: RadixThemesButton,
Flex: RadixThemesFlex,
Text: RadixThemesText,
} = window.__reflex["@radix-ui/themes"];
const { EventLoopContext } = window["__reflex"]["$/utils/context"];
const { ReflexEvent, applyEventActions } = window["__reflex"]["$/utils/state"];
export default function MySSRComponent() {
const [addEvents, connectErrors] = useContext(EventLoopContext);
return jsx(
RadixThemesFlex,
{
align: "start",
className: "rx-Stack",
direction: "row",
justify: "center",
gap: "5",
},
jsx(
RadixThemesButton,
{
onClick: (_e) =>
addEvents(
[
ReflexEvent(
"reflex___state____state.repro_dynamic_component___repro_dynamic_component____state.set_count",
{ ["count"]: 0 },
{},
),
],
[_e],
{},
),
},
"-",
),
jsx(RadixThemesText, { as: "p", size: "9" }, 1),
jsx(
RadixThemesButton,
{
onClick: (_e) =>
addEvents(
[
ReflexEvent(
"reflex___state____state.repro_dynamic_component___repro_dynamic_component____state.set_count",
{ ["count"]: 2 },
{},
),
],
[_e],
{},
),
},
"+",
),
);
} |
evalReactComponent returned the module's default export (a component function) directly, so the runtime tried to render the function as a child rather than instantiating it — event handlers on the dynamic subtree never wired up. Wrap the default export in React.createElement so the dynamic component mounts as a real element. Adds an integration regression for a counter-style dynamic component that exercises event handlers on the dynamically rendered subtree.
…test The codegen-level bug those changes were meant to address (dynamic components emitting JSX without onClick / EventLoopContext wiring) was already fixed by 3f08dd7 ("Do not auto-memoize components with imports-only VarData"). The state.js change was a no-op for the reported symptom and freezes the dynamic element with no props at eval time, which breaks any consumer expecting a renderable component. Drop the runtime change and the integration test additions; keep the codegen unit tests as a regression guard for the memoize fix.
useState's setter treats a function argument as an updater (setState(prev => newValue)), so passing module.default (a function) to set_xdvxrcsn invokes MySSRComponent inside basicStateReducer, where its useContext / useRef hooks run outside a render cycle and trip "Rendered more hooks than during the previous render." Wrapping in React.createElement returns an element object — useState stores it directly without invoking it as an updater, and React renders the element normally on the next pass, running hooks in a proper render context. Reverts 4d74b68. Keeps the codegen unit tests (independently useful as a regression guard for the auto-memoize fix in 3f08dd7).
Allows better modularity in `evalReactComponent` to not assume the callers usage of the returned value at that time.
FarhanAliRaza
force-pushed
the
compiler-hooks
branch
from
d881721 to
67a450d
Compare
2 participants
Summary
enter_component/leave_component/eval_page/compile_pagehooks, enabling third-party plugins to participate in the frontend compilation pipelineApp._compile()intocompiler.compile_app(), decoupling compilation logic from the App classExecutorType/ExecutorSafeFunctionsabstractions fromenvironment.pyin favor of a sequential plugin-driven compilation modelDefaultPagePlugin,ApplyStylePlugin,DefaultCollectorPluginthat replicate the existing compilation behaviorDefaultPagePluginto preserveVar-backed page titles instead of replacing them with the default stringKey Changes
packages/reflex-core/src/reflex_core/plugins/compiler.py(new): Core plugin infrastructure —CompilerPluginprotocol,CompileContext,PageContext,CompilerHooks, and component tree traversal logicreflex/compiler/plugins/builtin.py(new): Built-in plugins (DefaultPagePlugin,ApplyStylePlugin,DefaultCollectorPlugin) that replicate existing compilation behaviorreflex/compiler/compiler.py: Refactoredcompile_app()to use plugin-driven compilation viaCompilerHooksreflex/app.py: Removed ~430 lines of compilation logic,App._compile()now delegates tocompiler.compile_app()packages/reflex-core/src/reflex_core/environment.py: RemovedExecutorTypeandExecutorSafeFunctions(no longer needed)Test Plan
tests/units/compiler/test_plugins.py(855 lines) covering:CompileContextlifecycle and state managementDefaultPagePluginbehavior including Var-backed titlesApplyStylePluginstyle applicationDefaultCollectorPluginimport/custom code collectiontests/units/test_app.pyverifying memo components are written to shared components moduleAll Submissions:
Type of change
Please delete options that are not relevant.
New Feature Submission:
Changes To Core Features:
closes #6210
closes #6211
closes #6212
Closes #6213
Closes #6325