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

Minify state names v2 #3728

Open
wants to merge 27 commits into
base: main
Choose a base branch
from

Conversation

benedikt-bartscher
Copy link
Contributor

@benedikt-bartscher benedikt-bartscher commented Jul 31, 2024

Alternative to #3701

@benedikt-bartscher
Copy link
Contributor Author

benedikt-bartscher commented Aug 4, 2024

Actually, it's not a problem with the env vars not getting passed. Tests are failing due to _var_set_state which is called during init_subclass hooks and writes the state names into the vars/vardata.

@benedikt-bartscher
Copy link
Contributor Author

benedikt-bartscher commented Aug 5, 2024

Actually, it's not a problem with the env vars not getting passed. Tests are failing due to _var_set_state which is called during init_subclass hooks and writes the state names into the vars/vardata.

@masenf do you have any idea how to solve this?
I have added a skip marker to those tests. They are working fine if you set REFLEX_MINIFY_STATES or prod env in your shell.

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.

as for the _var_set_state, we probably could just save a reference to the state class and defer formatting of the state name until it's actually needed (_var_full_name_needs_state_prefix)

@benedikt-bartscher
Copy link
Contributor Author

as for the _var_set_state, we probably could just save a reference to the state class and defer formatting of the state name until it's actually needed (_var_full_name_needs_state_prefix)

Thanks, i was thinking about that. Do you want me to migrate to state refs in this PR or in a followup?

@masenf
Copy link
Collaborator

masenf commented Aug 5, 2024

i think you can do the refs in this PR, as long as it's not too invasive. We have a big Var refactor coming in the next week or so, that might conflict, but we can fix that up

@benedikt-bartscher benedikt-bartscher marked this pull request as draft September 4, 2024 19:38
@masenf
Copy link
Collaborator

masenf commented Oct 8, 2024

Merged origin/main and made a few fixes on top to get the tests working for me locally.

@benedikt-bartscher benedikt-bartscher marked this pull request as ready for review October 10, 2024 21:12
@benedikt-bartscher
Copy link
Contributor Author

Merged origin/main and made a few fixes on top to get the tests working for me locally.

Thank you very much @masenf!

Regarding your commit msg "test_minified_states: remove skip -- things seem to be working as-is" - that might be due to the var refactor from @adhami3310 especially ComputedVar.__get__.

I think this should be ready for review ✔️

@benedikt-bartscher
Copy link
Contributor Author

Do we still want to get this in? I will rebase it if there is intention to review it

@masenf
Copy link
Collaborator

masenf commented Oct 24, 2024

yes, we want to get it 🤞 thanks @benedikt-bartscher

@Lendemor
Copy link
Collaborator

Marking this as Draft until the conflict with main are solved.
(Yes we want that PR 👍 )

@Lendemor Lendemor marked this pull request as draft October 25, 2024 18:16
@benedikt-bartscher benedikt-bartscher marked this pull request as ready for review October 29, 2024 00:07
@benedikt-bartscher
Copy link
Contributor Author

Hi @Lendemor and @masenf good to hear that you want this 🚀
I resolved all the conflicts, we just need to coordinate the env vars with #4248 otherwise this PR is ready for review

@Lendemor
Copy link
Collaborator

Thanks for your work on this.
I believe we should work on getting #4248 merged first, then adapt this one to work with it.

@benedikt-bartscher
Copy link
Contributor Author

Thanks for your work on this. I believe we should work on getting #4248 merged first, then adapt this one to work with it.

You are welcome. Sounds good, I will wait for a review of #4248 then

@benedikt-bartscher benedikt-bartscher force-pushed the minify-state-names-v2 branch 2 times, most recently from 5e7c38c to 7652fa1 Compare November 1, 2024 21:55
@benedikt-bartscher
Copy link
Contributor Author

@Lendemor I based this onto #4248 and migrated the minify states env to the new env system to speed up the process

@masenf
Copy link
Collaborator

masenf commented Nov 5, 2024

rebased on main after 4248, will do some final review and testing. planning to merge today

@benedikt-bartscher
Copy link
Contributor Author

Thanks, @masenf! I checked the diff again, looks good to me

reflex/testing.py Outdated Show resolved Hide resolved
tests/integration/conftest.py Outdated Show resolved Hide resolved
@masenf
Copy link
Collaborator

masenf commented Nov 5, 2024

this patch doesn't seem to work anymore. the state names in context.js have a mix of full and minified names and i'm getting key errors left and right.

export const clientStorage = {"cookies": {"reflex___state____state.U.my_cookie": {"path": "/", "sameSite": "lax"}, "reflex___state____state.U.custom_cookie": {"name": "CustomNamedCookie", "path": "/", "maxAge": 3600, "sameSite": "lax"}}, "local_storage": {"reflex___state____state.U.my_local_storage": {"sync": false}, "reflex___state____state.B.data_raw": {"sync": false}}, "session_storage": {}}

export const main_state_name = "cQ"
export const update_vars_internal = "cu.update_vars_internal"
export const state_name = "cQ"

export const exception_state_name = "reflex___state____state.cs"

i tried this on the commit before i rebased, 7652fa1

@benedikt-bartscher
Copy link
Contributor Author

@masenf thanks for testing it again
It seems like the prod mode env is not set early enough when using the cli (reflex run --env prod)

Traceback (most recent call last):
  File "/.cache/pypoetry/virtualenvs/minified-state-names-IlpRchCp-py3.13/bin/reflex", line 5, in <module>
    from reflex.reflex import cli
  File "/reflex/reflex/reflex.py", line 18, in <module>
    from reflex.state import reset_disk_state_manager
  File "/reflex/reflex/state.py", line 2228, in <module>
    class State(BaseState):
    ...<3 lines>...
        is_hydrated: bool = False
  File "/.cache/pypoetry/virtualenvs/minified-state-names-IlpRchCp-py3.13/lib/python3.13/site-packages/pydantic/v1/main.py", line 282, in __new__
    cls = super().__new__(mcs, name, bases, new_namespace, **kwargs)
  File "<frozen abc>", line 106, in __new__
  File "/reflex/reflex/state.py", line 576, in __init_subclass__
    f.name: get_var_for_field(cls, f)
            ~~~~~~~~~~~~~~~~~^^^^^^^^
  File "/reflex/reflex/state.py", line 277, in get_var_for_field
    field_name = format.format_state_name(cls.get_full_name()) + "." + f.name
                                          ~~~~~~~~~~~~~~~~~^^
  File "/reflex/reflex/state.py", line 951, in get_full_name
    name = cls.get_name()
  File "/reflex/reflex/state.py", line 935, in get_name
    raise Exception(
        f"{EnvironmentVariables.REFLEX_ENV_MODE.get()=}\n{EnvironmentVariables.REFLEX_MINIFY_STATES.get()=}"
    )
Exception: EnvironmentVariables.REFLEX_ENV_MODE.get()=<Env.DEV: 'dev'>
EnvironmentVariables.REFLEX_MINIFY_STATES.get()=False

@benedikt-bartscher
Copy link
Contributor Author

benedikt-bartscher commented Nov 6, 2024

@masenf I do not really like it but with 6ce9471 it works
init_subclass magic hitting again :/

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.

3 participants