Explicit state and event id minification#6100
Explicit state and event id minification#6100benedikt-bartscher wants to merge 35 commits intoreflex-dev:mainfrom
Conversation
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
| has_substates = len(substates) > 0 | ||
|
|
||
| if handlers: | ||
| console.log(f"{child_prefix}|-- Event Handlers:") |
There was a problem hiding this comment.
Very tiny optical nit, cause sometimes EventHandlers is the last sub item.
It might work like this, didn't test it.
| console.log(f"{child_prefix}|-- Event Handlers:") | |
| console.log(f"{child_prefix}{'|' if has_substates else '`'}-- Event Handlers:") |
|
TODO: best-effort-mode
a similar thing can be implemented for event handler id's edit: not that easy, as state names are baked into to many things (events, vars etc). we would need to defer init_subclass and possibly parts of @rx.var and @rx.event edit2: i have found a better approach with a minify.json which makes this obsolete |
|
Open for discussion: Should reflex reserve the first 5/10 state id's for internal states? Current reflex uses 0-3, but that might change in the future. edit: with sibling uniqueness we would just need to reserve the state id's on the first rx.State subclass level. edit2: i have found a better approach with a minify.json which makes this obsolete |
|
TODO: think about sibling uniqueness for states. currently using global uniqueness edit: adjusted in 6b4c5fa |
35ad1c6 to
1647c1e
Compare
|
Idea: store ids in a minify.json file
|
When a parent and child state have the same minified name, substate resolution can fail because the leading segment is stripped incorrectly. This change adds a flag to skip stripping only on the initial recursive call, ensuring correct resolution even in name collision scenarios.
| fill: Var[str | Color] | ||
|
|
||
| # The stroke color of brush | ||
| stroke: Var[str | Color] |
Greptile OverviewGreptile SummaryThis change updates the The minify command group lives in Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
autonumber
participant User
participant CLI as reflex/reflex.py (click)
participant Prereq as reflex.utils.prerequisites
participant State as reflex.state.State
participant Minify as reflex.minify
participant FS as minify.json
User->>CLI: reflex minify sync/validate/init
CLI->>Prereq: get_app() (register State subclasses)
Prereq->>State: import app / build state tree
CLI->>Minify: generate/sync/validate config
Minify->>FS: read/write minify.json
Minify-->>CLI: config/errors/warnings
CLI-->>User: log counts/results
|
masenf
left a comment
There was a problem hiding this comment.
just started reviewing this, will need to test on it a bit more.
the explicit management of the minified names via cli solves our earlier problems of mismatching the minified names between compile and build time, but it seems like a lot of extra api surface area for a mostly under the covers feature.
would it make sense to auto-minify and store the minify.json as a compilation artifact in the .web/backend directory? we already do something similar to save which pages need to be re-evaluated in backend-only mode in order to have a consistent state tree.
reflex/utils/format.py
Outdated
| assert isinstance(handler, EventHandler), ( | ||
| f"Expected EventHandler, got {type(handler)}" | ||
| ) |
There was a problem hiding this comment.
assertions in non-test code might be optimized out if one runs with python -O
since this runs as part of the compiler, it's better to check the condition and (in this case) raise TypeError
There was a problem hiding this comment.
this is mainly used to satisfy the type checker. A pattern which i personally prefer over cast. At runtime this should always be EventHandler. However, no strong preference, i just changed it to raise instead
| raise SystemExit(1) | ||
|
|
||
| # Load the user's app to register all state classes | ||
| prerequisites.get_app() |
There was a problem hiding this comment.
just getting the app might not be sufficient to slurp all of the states. some states, like ComponentState or dynamically created states might only be created while evaluating pages.
maybe it's okay to leave those out of the minification?
There was a problem hiding this comment.
yeah, i was aware of this limitation, maybe i can figure it out later. I'd love to support ComponentState as well
|
Thanks you @masenf!
Yeah, it is. Advanced users and big deployments might prefer managing this manually. I think about it like db migrations. Manual management makes it possible to rename states and event handlers without any issues in production.
The minify.json should be persisted/commited to have stable id's. However if we decide that minify becomes default, we can certainly auto-update the minify.json during |
Summary
Add state and event name minification with CLI management tools.
minify.jsonconfig file to map state/event names to short IDs (e.g.,myapp.state.AppState→"a")reflex minifyCLI command group for managing minificationCLI Commands
based on #6098