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-1052/rx.State as Base State #2146

Merged
merged 26 commits into from
Nov 29, 2023

Conversation

ElijahAhianyo
Copy link
Collaborator

@ElijahAhianyo ElijahAhianyo commented Nov 8, 2023

This PR renames rx.State to rx.BaseState and creates a base state(rx.State) for all client apps. With this approach every substate will be a child(or substate) of rx.State. Also allows multiple inheritance from rx.State:

import reflex as rx

class FirstState(rx.State):
  ...

class SecondState(rx.State):
   ...

docs: reflex-dev/reflex-web#306

@ElijahAhianyo ElijahAhianyo force-pushed the REF-1052/Handle_rx.state_multiple_substates branch 2 times, most recently from c93db07 to 60cc30f Compare November 21, 2023 06:33
@ElijahAhianyo ElijahAhianyo force-pushed the REF-1052/Handle_rx.state_multiple_substates branch 2 times, most recently from 023d40a to 02ba981 Compare November 27, 2023 15:22
@ElijahAhianyo ElijahAhianyo marked this pull request as ready for review November 27, 2023 17:10
reflex/state.py Outdated Show resolved Hide resolved
reflex/state.py Outdated Show resolved Hide resolved
reflex/constants/base.py Outdated Show resolved Hide resolved
integration/test_client_storage.py Outdated Show resolved Hide resolved
@ElijahAhianyo ElijahAhianyo force-pushed the REF-1052/Handle_rx.state_multiple_substates branch from f7e57b9 to ca82705 Compare November 28, 2023 10:54
ElijahAhianyo and others added 4 commits November 28, 2023 10:55
Co-authored-by: Masen Furer <m_github@0x26.net>
Co-authored-by: Masen Furer <m_github@0x26.net>
Only track direct subclasses of each BaseState class
masenf
masenf previously approved these changes Nov 28, 2023
Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

a few more small things

if len(state_subclasses) > 1:
raise ValueError(
"rx.State has been subclassed multiple times. Only one subclass is allowed"
"rx.BaseState cannot be subclassed multiple times. use rx.State instead"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"rx.BaseState cannot be subclassed multiple times. use rx.State instead"
"rx.BaseState cannot be subclassed multiple times. Use rx.State instead."

Comment on lines +165 to +167
# reset rx.State subclasses
State.class_subclasses.clear()
# self.app_module.app.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# reset rx.State subclasses
State.class_subclasses.clear()
# self.app_module.app.
# Reset rx.State subclasses.
State.class_subclasses.clear()

config = get_config()
module = ".".join([config.app_name, config.app_name])
sys.path.insert(0, os.getcwd())
app = __import__(module, fromlist=(constants.CompileVars.APP,))
if reload:

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

When running AppHarness or unit tests via pytest, delete duplicate state names
(and basically redefine the state tree with the new class).

In a previous patch i removed this bit of code
(https://github.com/reflex-dev/reflex/pull/2227/files#diff-704efe440192781e072231490ac1d12defd1aba5f6debafbc7907eebcc2abcc7L304-L314).

But it turns out to be necessary. So I restructured the duplicate name check a
bit to indicate that this subclass resetting should only occur in testing
modes, and expand a bit on why we're doing it that way.
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.

Looks good to me!

@picklelo picklelo merged commit e3ee980 into main Nov 29, 2023
45 checks passed
@masenf masenf changed the title RED-1052/rx.State as Base State REF-1052/rx.State as Base State Nov 30, 2023
@masenf masenf deleted the REF-1052/Handle_rx.state_multiple_substates branch December 6, 2023 22:16
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.

4 participants