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.

Sign up for GitHub

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

Enhance Local State Management with Instant Feedback via Client-Side Input Tag #148

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

Enhance Local State Management with Instant Feedback via Client-Side Input Tag #148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
070e511
implemented a new input component that manage state on the client side
niikkhilsharma Mar 10, 2024
736b9fd
indentation error fixed
niikkhilsharma Mar 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions nextpy/interfaces/templates/jinja/web/pages/base_page.js.jinja2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Purpose:
{# ==============================
LIBRARY IMPORTS BLOCK
============================== #}
import { DispatchContext } from 'utils/context'
{#
Purpose:
- Renders all required library imports for the current page or component.
Expand Down
6 changes: 5 additions & 1 deletion nextpy/interfaces/web/components/component.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1589,6 +1589,9 @@ def _get_memoized_event_triggers(
rendered_chain = format.format_prop(event)
if isinstance(rendered_chain, str):
rendered_chain = rendered_chain.strip("{}")

if tag_name.startswith("Input_"):
rendered_chain = rendered_chain+'}'
Comment on lines +1592 to +1594
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The conditional addition of a closing brace } to rendered_chain based on the tag_name starting with "Input_" seems like a workaround for a specific case. This approach might introduce maintenance challenges and unexpected behavior for future components or changes.

Consider implementing a more generic solution that ensures rendered_chain is correctly formatted for all components, possibly by reviewing how rendered_chain is constructed and used across different component types.

Copy link

Choose a reason for hiding this comment

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

+1 to this, why do we do this? Instead, this should have been handled basis value prop as we discussed initially. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

@akshatbhargava123, you're right to question the specific handling for components starting with "Input_". This was indeed a workaround for a particular case, which might not be the best approach for maintainability and consistency across different components.

The initial idea to handle this based on the value prop could provide a more generic and robust solution. This approach would ensure that rendered_chain is correctly formatted for all components, focusing on the actual properties and behavior of the components rather than their naming conventions.

I'll review the implementation details and consider how we can adjust this to align with the initial discussion about the value prop. This might involve a more in-depth analysis of how rendered_chain is constructed and ensuring it's appropriately formatted for all scenarios, not just specific cases. Thank you for bringing this to attention, and I'll work on identifying a solution that adheres to our initial agreement and improves the code's maintainability and consistency.


# Hash the rendered EventChain to get a deterministic function name.
chain_hash = md5(str(rendered_chain).encode("utf-8")).hexdigest()
Expand All @@ -1613,7 +1616,8 @@ def _get_memoized_event_triggers(
Var.create_safe(memo_name)._replace(
_var_type=EventChain, merge_var_data=memo_var_data
),
f"const {memo_name} = useCallback({rendered_chain}, [{', '.join(var_deps)}])",
# Move the dispatchers line to the right place.
f"const dispatchers = useContext(DispatchContext);const {memo_name} = useCallback({rendered_chain}, [{', '.join(var_deps)}])",
)
return trigger_memo

Expand Down
29 changes: 28 additions & 1 deletion nextpy/utils/format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,7 @@ def format_match(cond: str | Var, match_cases: List[BaseVar], default: Var) -> s

def format_prop(
prop: Union[Var, EventChain, ComponentStyle, str],
tag_name='',
) -> Union[int, float, str]:
"""Format a prop.

Expand Down Expand Up @@ -349,7 +350,33 @@ def format_prop(

chain = ",".join([format_event(event) for event in prop.events])
event = f"addEvents([{chain}], {arg_def}, {json_dumps(prop.event_actions)})"
prop = f"{arg_def} => {event}"

if tag_name.startswith("Input_"):
Copy link

Choose a reason for hiding this comment

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

Same as the other instance of this? Why handle basis tag_name?

if isinstance(event, str):
event = event.strip("{}")

parts = chain.split('.')
formatted_chain = f'{parts[0]}.{parts[1]}.{parts[2]}'
Copy link

Choose a reason for hiding this comment

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

Could it be possible that parts array is empty? This would break then, right?

Comment on lines +355 to +359
Copy link

Choose a reason for hiding this comment

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

Added comment explaining what this part does and why.


# Extract "_e0.target.value"
value_match = re.search(r"value:([^,)}]+)", event)
if value_match:
value = value_match.group(1)

# Extract "state.state"
message_match = re.search(r"addEvents\(\[\S+?\(\"([^.]+?\.[^.]+)", event)
if message_match:
message = message_match.group(1)

dispatcher_line = f"const dispatcher = dispatchers['{message}'];\n" \
f"dispatcher({{ message: {value} }});"

prop = f"{arg_def} =>{{ {dispatcher_line}\n{event} }}"

Comment on lines +354 to +375
Copy link
Contributor

Choose a reason for hiding this comment

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

The conditional logic introduced for handling tags that start with "Input_" is innovative and aligns with the PR's objectives for client-side state management and instant feedback. However, there are several areas that could be refined for better readability, maintainability, and performance:

  1. String Manipulation Efficiency: The use of strip("{}") and split('.') for string manipulation can be inefficient and error-prone, especially for complex event chains. Consider using more robust parsing techniques or utilities designed for handling such patterns.

  2. Regular Expression Usage: The use of regular expressions to extract values and messages is a powerful technique but can be hard to maintain and understand for someone not familiar with the specific patterns being matched. Ensure that these expressions are well-documented with comments explaining their purpose and the expected format of the strings they are applied to.

  3. Dispatcher Line Construction: The construction of the dispatcher_line variable is a critical part of the logic for handling "Input_" tags. It's important to ensure that this line is constructed correctly and efficiently. Consider validating the existence of the dispatcher and handling potential errors gracefully to avoid runtime exceptions.

  4. Prop Construction: The final construction of the prop variable for "Input_" tags involves a complex string manipulation that could benefit from a more structured approach. Consider using template strings or other formatting techniques to make this code more readable and less prone to errors.

Overall, while the approach taken here is aligned with the PR's objectives, refining these aspects could greatly improve the code's quality and maintainability.

# Handle other types.
Copy link

Choose a reason for hiding this comment

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

What other types? Ideally you should just create a string in the conditional and then inject that string in prop as assigned below. What do you think?

else:
# prop = f"{arg_def} =>{{ {dispatcher_line}\n{event} }}"
Copy link

Choose a reason for hiding this comment

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

Remove if not required.

prop = f"{arg_def} => {event}"

# Handle other types.
elif isinstance(prop, str):
Expand Down
Loading