Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REF-889] useContext per substate #2149

Merged
merged 36 commits into from Nov 21, 2023
Merged

Conversation

masenf
Copy link
Collaborator

@masenf masenf commented Nov 8, 2023

This PR lays the groundwork for independently operating substates in their own contexts. Why would we want this? Because components that only depend on a substate do not need to re-render when some unrelated part of the state changes.

Fix REF-889 Separate context per child state

The state dict() and get_delta() functions now both return data in the same format:

{
    state.get_full_name(): all_vars,
    ...
}

This flat structure is easier to work with on both ends, and now the wire format of both full dict and partial delta look the same and can be used interchangably.

In contexts.js.jinja2, the code now iterates through the initialState and creates a separate context with reducer+dispatch for each top-level key.

In state.js when an update comes in, look up the associated dispatcher and apply the subdelta to it. applyDelta becomes a dead simple spread operation now that each substate is just a flat dictionary.

Fix REF-1128 Var should have imports and hooks

Also Fix REF-564 Var operations should carry state.

Now each Var has a _var_data field which stores information about what hooks and imports are needed in any component that includes that Var. When rendering a component, there is a new _get_vars iterator that yields all vars used by the component. Any hooks or imports collected from these are added to the components hooks/imports.

This has allowed the removal of nearly all default imports in reflex/compiler/compiler.py, since the Vars that use particular imports now carry that information themselves.

With the addition of _var_data comes _replace method, which is used internally by Var operations to make a copy of the Var with some attributes overwritten. This helps preserve state and var_data across operations.

Because _var_data.imports and _var_data.hooks are collections, operations involving multiple vars will accumulate imports and hooks, allowing for arbitrary combination of vars without losing this dependency information.

This structure will also enable more advanced Var use cases, such as wrapping component-local useState hooks for client-side only component state.

Other Changes

ImportVar was moved into reflex.utils.imports to reduce annoyances around circular imports with constants and var modules.

Var has _var_hooks and _var_imports

Instead of relying on the page/component having
the appropriate imports statically, allow these to
be dynamically calculated for greater flexibility
Ensure that adding additional Var metadata in the
future does not require lots of changes everywhere,
by placing all carryable metadata into _var_data
Remove all* default imports for a page, instead relying
on the components and vars that use a particular
import to name it explicitly with an ImportVar

Remove hardcoded hooks, instead format these in for
components that depend on them.

Move ImportVar to reflex.utils.imports so it can live
next to the ImportDict and avoid circular import with
reflex.vars
It needs to render with curly braces wrapped around it
Instead avoid rendering triggers in Component.render,
but otherwise leave them alone so that components
may be rendered multiple times without changing the
output.
Using `rx.cond` helper ensures that the condition carries the `isTrue` import.
Do not surround the formatted values with extra curly braces
reflex/vars.py Show resolved Hide resolved
Lendemor
Lendemor previously approved these changes Nov 15, 2023
Ensure that Vars used in different contexts in a Component are correctly
identified and returned.
@masenf
Copy link
Collaborator Author

masenf commented Nov 16, 2023

@Lendemor @picklelo @ElijahAhianyo this one is fixed up and ready to the best of my knowledge

`format_cond` doesn't really work with f-string semantics due to a blind `{`,
`}` removal which breaks the JSON-encoding of VarData. So instead, we carry the
VarData explicitly on the cond_var and change `format_cond` to `str` the Var
before formatting to ensure it does not emit a VarData-encoded f-string.
Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code works great for me, but left some comments on code style.

I think for any non-trivial function we need to consistently block the code into chunks separated by newlines, where each chunk has a full sentence comment. Not necessarily breaking out into sub functions, but keeping individual function code clean by segmenting it whenever we have conditionals, loops, or pieces of logic, and clearly commenting each of these segments with a comment. Then it can also be easy to validate of the code is doing what the comment says. As we add more complex logic, it will help keep the code more readable and maintainable.

reflex/compiler/compiler.py Show resolved Hide resolved
reflex/components/component.py Outdated Show resolved Hide resolved
reflex/components/component.py Outdated Show resolved Hide resolved
reflex/components/component.py Show resolved Hide resolved

yield from self.special_props

for comp_prop in (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't style go in here also?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.style itself will never be a Var, but it might have Var values inside of it, which is why it's handled specially.

reflex/style.py Show resolved Hide resolved
reflex/style.py Outdated Show resolved Hide resolved
reflex/style.py Show resolved Hide resolved
reflex/style.py Show resolved Hide resolved
reflex/vars.py Show resolved Hide resolved
@masenf
Copy link
Collaborator Author

masenf commented Nov 21, 2023

I merged main back into this branch after the compile optimization PR merged and these changes regress compile time by 3 seconds total. Will need to poke around with the profiler and see if i can speed up these changes.

Edit: was able to shrink the regression down to only ~1.5 - 2s on reflex-web

* Do not cast to Style() early in `Component.add_style`
* Memoize return value of `Component._get_vars`
* Defer `VarData.merge` for most operations -- call it once
* Avoid `serializers.serialize` for primitive JSON types
reflex/components/component.py Show resolved Hide resolved
reflex/components/layout/cond.py Outdated Show resolved Hide resolved
@picklelo picklelo merged commit 1603144 into main Nov 21, 2023
45 checks passed
@masenf masenf deleted the masenf/context-per-substate-rb branch November 21, 2023 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants