Skip to content

raise when ids are vars#5783

Closed
adhami3310 wants to merge 1 commit intomainfrom
raise-when-ids-are-vars
Closed

raise when ids are vars#5783
adhami3310 wants to merge 1 commit intomainfrom
raise-when-ids-are-vars

Conversation

@adhami3310
Copy link
Member

No description provided.

@adhami3310 adhami3310 linked an issue Sep 11, 2025 that may be closed by this pull request
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Summary

This PR implements validation to prevent component IDs from being Var objects in the Reflex framework. The change adds strict checking in the Component class's _create and _unsafe_create methods to ensure that IDs are always strings and cannot contain the REFLEX_VAR_OPENING_TAG marker that indicates a Var expression.

The validation logic checks two conditions: first, that the ID is a string type, and second, that it doesn't contain the Reflex Var opening tag. If either condition fails, a TypeError is raised with the message "Component id must be a string and cannot be a Var." This prevents runtime issues in the generated React code where HTML element IDs must be strings for proper DOM manipulation and component referencing.

The change also updates the type annotation for the id parameter from Any to str | None, improving type safety. The corresponding test cases that previously validated Var IDs are removed since they would now fail due to the new validation. Finally, all Python stub files (.pyi) are regenerated and their hashes updated in pyi_hashes.json to maintain consistency with the modified component interface.

This change enforces a more restrictive but safer approach to component IDs, ensuring they remain static strings rather than dynamic expressions that could cause frontend issues.

Confidence score: 4/5

  • This PR is safe to merge with minimal risk as it adds validation that prevents potential runtime errors
  • Score reflects well-implemented validation logic with proper error handling, though it introduces a breaking change for existing code using Var IDs
  • Pay close attention to any existing code that might be using Var objects as component IDs, as this will now raise errors

3 files reviewed, no comments

Edit Code Review Bot Settings | Greptile

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 11, 2025

CodSpeed Performance Report

Merging #5783 will not alter performance

Comparing raise-when-ids-are-vars (8c11a10) with main (f0b2075)

Summary

✅ 8 untouched

@adhami3310 adhami3310 closed this Sep 11, 2025
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.

would it still be possible to allow Var for id, but if ref is not False, then we emit a deprecation warning saying something like the id must be a real string to create a ref, set ref=False to silence this warning. then in a subsequent release (maybe 0.9) we can actually fix id/ref handling in a more graceful way.

i just don't want to break an app that was compiling and working if we can avoid it.

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.

Upload id should be able to be a var

2 participants