From 5fae1bb5638692a939d6a6403dc86067c442a8f1 Mon Sep 17 00:00:00 2001 From: Masen Furer Date: Thu, 14 Mar 2024 11:54:31 -0700 Subject: [PATCH 1/6] CustomComponent ignores the annotation type in favor of the passed value Do not require rx.memo wrapped functions to have 100% correct annotations. --- reflex/components/component.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/reflex/components/component.py b/reflex/components/component.py index 71b870b8c..a81e94178 100644 --- a/reflex/components/component.py +++ b/reflex/components/component.py @@ -1296,17 +1296,12 @@ class CustomComponent(Component): self.props[format.to_camel_case(key)] = value continue - # Convert the type to a Var, then get the type of the var. - if not types._issubclass(type_, Var): - type_ = Var[type_] - type_ = types.get_args(type_)[0] - # Handle subclasses of Base. - if types._issubclass(type_, Base): + if isinstance(value, Base): base_value = Var.create(value) # Track hooks and imports associated with Component instances. - if base_value is not None and types._issubclass(type_, Component): + if base_value is not None and isinstance(value, Component): value = base_value._replace( merge_var_data=VarData( # type: ignore imports=value.get_imports(), From 3121e8a7a9e74845c7c99765c0ef86c4f9a2e3f6 Mon Sep 17 00:00:00 2001 From: Masen Furer Date: Thu, 14 Mar 2024 16:16:29 -0700 Subject: [PATCH 2/6] test_custom_component_get_imports: test imports from wrapped custom components --- tests/components/test_component.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/components/test_component.py b/tests/components/test_component.py index 04468b2ba..d5413229f 100644 --- a/tests/components/test_component.py +++ b/tests/components/test_component.py @@ -1269,3 +1269,28 @@ def test_deprecated_props(capsys): assert "type={`type1`}" in c2_1_render["props"] assert "min={`min1`}" in c2_1_render["props"] assert "max={`max1`}" in c2_1_render["props"] + + +def test_custom_component_get_imports(): + class Inner(Component): + tag = "Inner" + library = "inner" + + class Other(Component): + tag = "Other" + library = "other" + + @rx.memo + def wrapper(): + return Inner.create() + + @rx.memo + def outer(c: Component): + return Other.create(c) + + custom_comp = wrapper() + assert "inner" in custom_comp.get_imports() + + outer_comp = outer(c=wrapper()) + assert "inner" in outer_comp.get_imports() + assert "other" in outer_comp.get_imports() From 86a1e8a07e73dc468d5bcac0c2c96d96645c513d Mon Sep 17 00:00:00 2001 From: Masen Furer Date: Thu, 14 Mar 2024 16:24:12 -0700 Subject: [PATCH 3/6] Account for imports of custom components for frontend installation Create new ImportVar that only install the package to avoid rendering the imports in the page that contains the custom component itself. --- reflex/components/component.py | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/reflex/components/component.py b/reflex/components/component.py index a81e94178..e33f4e26c 100644 --- a/reflex/components/component.py +++ b/reflex/components/component.py @@ -1265,6 +1265,9 @@ class CustomComponent(Component): # The props of the component. props: Dict[str, Any] = {} + # Props that reference other components. + component_props: Dict[str, Component] = {} + def __init__(self, *args, **kwargs): """Initialize the custom component. @@ -1302,6 +1305,7 @@ class CustomComponent(Component): # Track hooks and imports associated with Component instances. if base_value is not None and isinstance(value, Component): + self.component_props[key] = value value = base_value._replace( merge_var_data=VarData( # type: ignore imports=value.get_imports(), @@ -1335,6 +1339,25 @@ class CustomComponent(Component): """ return hash(self.tag) + def _get_imports(self) -> Dict[str, List[ImportVar]]: + """Get the imports for the component. + + This is needed because otherwise the imports for the component are not + installed during compile time, but they are rendered into the page. + + Returns: + The imports for the component and any custom component props. + """ + return imports.merge_imports( + super()._get_imports(), + # Sweep up any imports from CustomComponent props for frontend installation. + { + library: [ImportVar(tag=None, render=False, install=True)] + for comp in self.get_custom_components() + for library in comp.get_component(comp).get_imports() + }, + ) + @classmethod def get_props(cls) -> Set[str]: """Get the props for the component. @@ -1368,6 +1391,16 @@ class CustomComponent(Component): custom_components |= self.get_component(self).get_custom_components( seen=seen ) + + # Fetch custom components from props as well. + for child_component in self.component_props.values(): + if child_component.tag is None: + continue + if child_component.tag not in seen: + seen.add(child_component.tag) + if isinstance(child_component, CustomComponent): + custom_components |= {child_component} + custom_components |= child_component.get_custom_components(seen=seen) return custom_components def _render(self) -> Tag: From f3ef8b280e82f56ab54524dbc121812fd3859448 Mon Sep 17 00:00:00 2001 From: Masen Furer Date: Thu, 14 Mar 2024 18:07:58 -0700 Subject: [PATCH 4/6] Handle Imports that should never be installed --- reflex/components/component.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/reflex/components/component.py b/reflex/components/component.py index e33f4e26c..042106bad 100644 --- a/reflex/components/component.py +++ b/reflex/components/component.py @@ -1352,9 +1352,15 @@ class CustomComponent(Component): super()._get_imports(), # Sweep up any imports from CustomComponent props for frontend installation. { - library: [ImportVar(tag=None, render=False, install=True)] + library: [ + ImportVar( + tag=None, + render=False, + install=not any(not imp.install for imp in imps), + ), + ] for comp in self.get_custom_components() - for library in comp.get_component(comp).get_imports() + for library, imps in comp.get_component(comp).get_imports().items() }, ) From 67624b48df760d7ffc830898fa5bce6f1a9cba49 Mon Sep 17 00:00:00 2001 From: Masen Furer Date: Thu, 14 Mar 2024 18:48:40 -0700 Subject: [PATCH 5/6] Fix up double negative logic --- reflex/components/component.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reflex/components/component.py b/reflex/components/component.py index 042106bad..a26ded175 100644 --- a/reflex/components/component.py +++ b/reflex/components/component.py @@ -1356,7 +1356,7 @@ class CustomComponent(Component): ImportVar( tag=None, render=False, - install=not any(not imp.install for imp in imps), + install=any(imp.install for imp in imps), ), ] for comp in self.get_custom_components() From e13ff71c2c9da6c0c629d6246e376bea761d57c0 Mon Sep 17 00:00:00 2001 From: Masen Furer Date: Thu, 14 Mar 2024 20:18:27 -0700 Subject: [PATCH 6/6] Perf Optimization: use the imports we already calculate during compile Instead of augmenting _get_imports with a weird, slow, recursive crawl and dictionary reconstruction, just use the imports that we compile into the components.js file to install frontend packages needed by the custom components. Same effect, but adds essentially zero overhead to the compilation. --- reflex/app.py | 15 ++++++++++----- reflex/compiler/compiler.py | 21 ++++++++++++++------- reflex/components/component.py | 25 ------------------------- tests/components/test_component.py | 20 +++++++++++++++++--- 4 files changed, 41 insertions(+), 40 deletions(-) diff --git a/reflex/app.py b/reflex/app.py index 5f1ef96f5..00a2d916b 100644 --- a/reflex/app.py +++ b/reflex/app.py @@ -818,6 +818,7 @@ class App(Base): compile_results.append((stateful_components_path, stateful_components_code)) result_futures = [] + custom_components_future = None def submit_work(fn, *args, **kwargs): """Submit work to the thread pool and add a callback to mark the task as complete. @@ -847,7 +848,10 @@ class App(Base): submit_work(compiler.compile_app, app_root) # Compile the custom components. - submit_work(compiler.compile_components, custom_components) + custom_components_future = thread_pool.submit( + compiler.compile_components, custom_components + ) + custom_components_future.add_done_callback(mark_complete) # Compile the root stylesheet with base styles. submit_work(compiler.compile_root_stylesheet, self.stylesheets) @@ -878,14 +882,15 @@ class App(Base): # Get imports from AppWrap components. all_imports.update(app_root.get_imports()) - # Iterate through all the custom components and add their imports to the all_imports. - for component in custom_components: - all_imports.update(component.get_imports()) - # Wait for all compilation tasks to complete. for future in concurrent.futures.as_completed(result_futures): compile_results.append(future.result()) + # Iterate through all the custom components and add their imports to the all_imports. + custom_components_result = custom_components_future.result() + compile_results.append(custom_components_result[:2]) + all_imports.update(custom_components_result[2]) + # Empty the .web pages directory. compiler.purge_web_pages_dir() diff --git a/reflex/compiler/compiler.py b/reflex/compiler/compiler.py index a3e6fb7fa..603c8b969 100644 --- a/reflex/compiler/compiler.py +++ b/reflex/compiler/compiler.py @@ -186,7 +186,9 @@ def _compile_component(component: Component) -> str: return templates.COMPONENT.render(component=component) -def _compile_components(components: set[CustomComponent]) -> str: +def _compile_components( + components: set[CustomComponent], +) -> tuple[str, Dict[str, list[ImportVar]]]: """Compile the components. Args: @@ -208,9 +210,12 @@ def _compile_components(components: set[CustomComponent]) -> str: imports = utils.merge_imports(imports, component_imports) # Compile the components page. - return templates.COMPONENTS.render( - imports=utils.compile_imports(imports), - components=component_renders, + return ( + templates.COMPONENTS.render( + imports=utils.compile_imports(imports), + components=component_renders, + ), + imports, ) @@ -401,7 +406,9 @@ def compile_page( return output_path, code -def compile_components(components: set[CustomComponent]): +def compile_components( + components: set[CustomComponent], +) -> tuple[str, str, Dict[str, list[ImportVar]]]: """Compile the custom components. Args: @@ -414,8 +421,8 @@ def compile_components(components: set[CustomComponent]): output_path = utils.get_components_path() # Compile the components. - code = _compile_components(components) - return output_path, code + code, imports = _compile_components(components) + return output_path, code, imports def compile_stateful_components( diff --git a/reflex/components/component.py b/reflex/components/component.py index a26ded175..b085ee352 100644 --- a/reflex/components/component.py +++ b/reflex/components/component.py @@ -1339,31 +1339,6 @@ class CustomComponent(Component): """ return hash(self.tag) - def _get_imports(self) -> Dict[str, List[ImportVar]]: - """Get the imports for the component. - - This is needed because otherwise the imports for the component are not - installed during compile time, but they are rendered into the page. - - Returns: - The imports for the component and any custom component props. - """ - return imports.merge_imports( - super()._get_imports(), - # Sweep up any imports from CustomComponent props for frontend installation. - { - library: [ - ImportVar( - tag=None, - render=False, - install=any(imp.install for imp in imps), - ), - ] - for comp in self.get_custom_components() - for library, imps in comp.get_component(comp).get_imports().items() - }, - ) - @classmethod def get_props(cls) -> Set[str]: """Get the props for the component. diff --git a/tests/components/test_component.py b/tests/components/test_component.py index d5413229f..8dfe6dedc 100644 --- a/tests/components/test_component.py +++ b/tests/components/test_component.py @@ -4,6 +4,7 @@ import pytest import reflex as rx from reflex.base import Base +from reflex.compiler.compiler import compile_components from reflex.components.base.bare import Bare from reflex.components.chakra.layout.box import Box from reflex.components.component import ( @@ -1289,8 +1290,21 @@ def test_custom_component_get_imports(): return Other.create(c) custom_comp = wrapper() - assert "inner" in custom_comp.get_imports() + + # Inner is not imported directly, but it is imported by the custom component. + assert "inner" not in custom_comp.get_imports() + + # The imports are only resolved during compilation. + _, _, imports_inner = compile_components(custom_comp.get_custom_components()) + assert "inner" in imports_inner outer_comp = outer(c=wrapper()) - assert "inner" in outer_comp.get_imports() - assert "other" in outer_comp.get_imports() + + # Libraries are not imported directly, but are imported by the custom component. + assert "inner" not in outer_comp.get_imports() + assert "other" not in outer_comp.get_imports() + + # The imports are only resolved during compilation. + _, _, imports_outer = compile_components(outer_comp.get_custom_components()) + assert "inner" in imports_outer + assert "other" in imports_outer