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 1 commit
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} }}"

Copy link
Contributor

Choose a reason for hiding this comment

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

The logic added for handling tag_name starting with "Input_" introduces a specialized way to manage state and provide feedback for input elements. This is a key part of the local input tag feature. However, there are a few points to consider:

  • Ensure that the split operation on chain (line 358) is robust against potential variations in the event chain format.
  • The extraction of value and message (lines 362-369) relies on regex, which might be fragile if the event format changes. Consider defining a more structured way to pass these values.
  • The construction of dispatcher_line (lines 371-372) and its inclusion in the final prop (line 374) is a clever way to inject state updates directly from input events. Ensure that this approach is consistent with the overall architecture and security practices, especially regarding XSS protection.

# 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