Repository navigation
Conversation
The enable_state=False guard only scanned page modules, but auto-memoization moves state reads into memo modules that are compiled afterwards. Run the guard once the memo modules exist so reads in memoized components, rx.memo functions, app wraps and the hydrate fallback are rejected too. Fixes reflex-dev#7579
Contributor
[Medium impact] The PR appears safe to merge; no blocking issues remain. Summary
Reviews (3) · Last reviewed commit: "test(compiler): start the state guard te..." · Reviewed by Greptile |
…isabled Wraps with MemoizationDisposition.NEVER keep their state reads in the app root, which compile_app_root produced after the guard ran. Run the guard after the app root is compiled and cover it with an unmemoized app wrap test.
Merging this PR will not alter performance1 warning
Comparing |
Memos that other modules define at import time can read state, and every registered memo is compiled. In a full unit run they tripped the guard in the stateless memo tests. Clear the memo table in isolated_compile.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
All Submissions:
Type of change
Changes To Core Features:
What
With
rx.App(enable_state=False),compile_appis supposed to raise "To access rx.State in frontend components, at least one subclass of rx.State must be defined in the app." when a component reads state. The check ran right after the pages were compiled and only looked at page output. Auto-memoization moves most state reads into memo modules, and those are compiled later, together with app-wrap memos and the hydrate fallback memo. So the issue's example (rx.text(DynState.label)) compiled without error and failed at runtime.The check now runs after
compile_memo_componentsand scans every module incompile_results, which at that point holds the page modules and all memo modules.code_uses_state_contextsitself is unchanged.Why this direction
I went for the smallest change that keeps the existing guard logic: same predicate, same error, just run over all the compiled JS that can subscribe to state instead of the pages only.
Alternatives I considered:
VarData.statefrom the compiled hooks (option 2). As @masenf pointed out, this misses the reported example, because_get_vars_hookskeeps only position metadata in the hook entries.Known limits, unchanged by this PR: the check still matches the
useStateContext(substring, so literal text containing it is still a false positive (now in memo modules too). One behavior change:MEMOSis compiled as a whole, so an@rx.memothat reads state is now rejected in anenable_state=Falseapp even if no page renders it. Before, it was emitted but never imported.Tests
New cases in
tests/units/compiler/test_compiler.py, using the realcompile_app(app, dry_run=True, use_rich=False)path:enable_state=Falsewith a state read raises for: an auto-memoized page component, a non-memoized page component, anrx.memobody, an app wrap, and the hydrate fallback.enable_state=Falsecompiles, in dev and in prod.Without the fix, 4 cases fail with
DID NOT RAISE ReflexRuntimeError(auto memo, explicit memo, app wrap, hydrate fallback); the non-memoized page case already raised. With the fix, all 12 pass. The new tests cover every changed line incompile_app.Also ran
test_var.py::test_code_uses_state_contexts,test_compiler.py,test_memoize_plugin.pyandtest_app.py(434 passed), andruff check,ruff format --check,pyrightandpre-commiton the changed files.closes #7579