-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Minify state names v2 #3728
Draft
benedikt-bartscher
wants to merge
18
commits into
reflex-dev:main
Choose a base branch
from
benedikt-bartscher:minify-state-names-v2
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Minify state names v2 #3728
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
594e426
wip minified state names
benedikt-bartscher 6e5f152
cleanup
benedikt-bartscher 24ff877
all _state_names should be classvars
benedikt-bartscher aefad4f
fix hardcoded event handlers and states
benedikt-bartscher 214def6
fix state name init for substates, thanks @masenf
benedikt-bartscher e7b273f
enable minified state names by default in prod
benedikt-bartscher e05ad13
add simple test that minified state names are unique
benedikt-bartscher 8760bd6
fix default state names
benedikt-bartscher 337f4cb
fix typo
benedikt-bartscher 3c1946d
wip minified state integration test
benedikt-bartscher bf6ec96
wip: more dynamic jinja contexts, tests for minification
benedikt-bartscher c913ec1
fix type ignore comment
benedikt-bartscher 4d2a72c
move state.js to jinja, related to #3738
benedikt-bartscher 6b1a73f
Revert "move state.js to jinja, related to #3738"
benedikt-bartscher 22a3a62
pass constants via js consts to state.js
benedikt-bartscher 4f43bb2
Merge remote-tracking branch 'upstream/main' into minify-state-names-v2
benedikt-bartscher 6074c96
Merge remote-tracking branch 'upstream/main' into minify-state-names-v2
benedikt-bartscher 33d0ef8
Merge remote-tracking branch 'upstream/main' into minify-state-names-v2
benedikt-bartscher File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,173 @@ | ||
"""Integration tests for minified state names.""" | ||
|
||
from __future__ import annotations | ||
|
||
import os | ||
from functools import partial | ||
from typing import Generator, Optional, Type | ||
|
||
import pytest | ||
from selenium.webdriver.common.by import By | ||
from selenium.webdriver.remote.webdriver import WebDriver | ||
|
||
from reflex.constants.compiler import ENV_MINIFY_STATES | ||
from reflex.testing import AppHarness, AppHarnessProd | ||
|
||
|
||
def TestApp(minify: bool | None) -> None: | ||
"""A test app for minified state names. | ||
|
||
Args: | ||
minify: whether to minify state names | ||
""" | ||
import reflex as rx | ||
|
||
class TestAppState(rx.State): | ||
"""State for the TestApp app.""" | ||
|
||
pass | ||
|
||
app = rx.App() | ||
|
||
def index(): | ||
return rx.vstack( | ||
rx.input( | ||
value=TestAppState.router.session.client_token, | ||
is_read_only=True, | ||
id="token", | ||
), | ||
rx.text(f"minify: {minify}", id="minify"), | ||
rx.text(TestAppState.get_name(), id="state_name"), | ||
rx.text(TestAppState.get_full_name(), id="state_full_name"), | ||
) | ||
|
||
app.add_page(index) | ||
|
||
|
||
@pytest.fixture( | ||
params=[ | ||
pytest.param(False), | ||
pytest.param(True), | ||
pytest.param(None), | ||
], | ||
) | ||
def minify_state_env( | ||
request: pytest.FixtureRequest, | ||
) -> Generator[Optional[bool], None, None]: | ||
"""Set the environment variable to minify state names. | ||
|
||
Args: | ||
request: pytest fixture request | ||
|
||
Yields: | ||
minify_states: whether to minify state names | ||
""" | ||
minify_states: Optional[bool] = request.param | ||
if minify_states is None: | ||
_ = os.environ.pop(ENV_MINIFY_STATES, None) | ||
else: | ||
os.environ[ENV_MINIFY_STATES] = str(minify_states).lower() | ||
yield minify_states | ||
if minify_states is not None: | ||
os.environ.pop(ENV_MINIFY_STATES, None) | ||
|
||
|
||
@pytest.fixture | ||
def test_app( | ||
app_harness_env: Type[AppHarness], | ||
tmp_path_factory: pytest.TempPathFactory, | ||
minify_state_env: Optional[bool], | ||
) -> Generator[AppHarness, None, None]: | ||
"""Start TestApp app at tmp_path via AppHarness. | ||
|
||
Args: | ||
app_harness_env: either AppHarness (dev) or AppHarnessProd (prod) | ||
tmp_path_factory: pytest tmp_path_factory fixture | ||
minify_state_env: need to request this fixture to set env before the app starts | ||
|
||
Yields: | ||
running AppHarness instance | ||
|
||
""" | ||
name = f"testapp_{app_harness_env.__name__.lower()}" | ||
with app_harness_env.create( | ||
root=tmp_path_factory.mktemp(name), | ||
app_name=name, | ||
app_source=partial(TestApp, minify=minify_state_env), # type: ignore | ||
) as harness: | ||
yield harness | ||
|
||
|
||
@pytest.fixture | ||
def driver(test_app: AppHarness) -> Generator[WebDriver, None, None]: | ||
"""Get an instance of the browser open to the test_app app. | ||
|
||
Args: | ||
test_app: harness for TestApp app | ||
|
||
Yields: | ||
WebDriver instance. | ||
|
||
""" | ||
assert test_app.app_instance is not None, "app is not running" | ||
driver = test_app.frontend() | ||
try: | ||
yield driver | ||
finally: | ||
driver.quit() | ||
|
||
|
||
def test_minified_states( | ||
test_app: AppHarness, | ||
driver: WebDriver, | ||
minify_state_env: Optional[bool], | ||
) -> None: | ||
"""Test minified state names. | ||
|
||
Args: | ||
test_app: harness for TestApp | ||
driver: WebDriver instance. | ||
minify_state_env: whether state minification is enabled by env var. | ||
|
||
""" | ||
assert test_app.app_instance is not None, "app is not running" | ||
|
||
is_prod = isinstance(test_app, AppHarnessProd) | ||
|
||
# default to minifying in production | ||
should_minify: bool = is_prod | ||
|
||
# env overrides default | ||
if minify_state_env is not None: | ||
should_minify = minify_state_env | ||
|
||
# TODO: reload internal states, or refactor VarData to reference state object instead of name | ||
if should_minify: | ||
pytest.skip( | ||
"minify tests are currently not working, because _var_set_states writes the state names during import time" | ||
) | ||
|
||
# get a reference to the connected client | ||
token_input = driver.find_element(By.ID, "token") | ||
assert token_input | ||
|
||
# wait for the backend connection to send the token | ||
token = test_app.poll_for_value(token_input) | ||
assert token | ||
|
||
state_name_text = driver.find_element(By.ID, "state_name") | ||
assert state_name_text | ||
state_name = state_name_text.text | ||
|
||
state_full_name_text = driver.find_element(By.ID, "state_full_name") | ||
assert state_full_name_text | ||
_ = state_full_name_text.text | ||
|
||
assert test_app.app_module | ||
module_state_prefix = test_app.app_module.__name__.replace(".", "___") | ||
# prod_module_suffix = "prod" if is_prod else "" | ||
|
||
if should_minify: | ||
assert len(state_name) == 1 | ||
else: | ||
assert state_name == f"{module_state_prefix}____test_app_state" |
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is already coming in as
state_name
.reflex/compiler/compiler.py:90
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iirc this was the state name for
rx.app.App.state
. will checkThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they are not the same.
compile_contexts
gets called fromreflex.app.App._compile
withself.state
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe for clarity, we should call the var
root_state_name
instead ofmain_state_name
just to indicate that it's the name ofrx.State
, the root of the state "tree"There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
main_state_dispatch
should we rename that one toroot_state_dispatch
as well?