Skip to content

Support Class Functions as React Props #221

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Conversation

omrimn
Copy link

@omrimn omrimn commented May 11, 2025

Overview

This PR enhances the function transform logic to support resolving functions defined as class methods on the custom element's container. Previously, only global or window-scoped functions could be referenced by name.

Now, if a function prop is passed as an attribute and matches a method on the element's container (using camelCase conversion), it will be correctly resolved and bound to the element.

@omrimn omrimn changed the title Support class function to react props Support Class Functions as React Props May 11, 2025
@bmomberger-bitovi
Copy link
Contributor

Thanks for your contribution, @omrimn . There are a few things that will need to be addressed before this feature can be added to the r2wc mainline.

  • Overloading the function prop converter to read off the element and its prototype chain would represent a breaking change. Even though it would be a rare case where the named property is unintentionally missing on the window/global but present on the element object, I don't want this to happen to anyone unintentionally. My recommendation is to copy the existing function transform to a new transform module, call it "method", and then we have a minor semver release since adding "method" to the prop transforms isn't a breaking change. Then we can have a constructive discussion after those releases on how to approach the "function" transform for core v2 and r2wc modern and legacy v3's.
  • All PRs that make changes to the API must be accompanied with changes to the documentation. We made a mistake by not having this policy recently, and it created a lot of tension. So however you work your PR to make it a non-breaking change, please update API.md so it describes. what your updated transform does.

@bmomberger-bitovi
Copy link
Contributor

I thought of another thing:

Your transformer isn't defining a property for the prop value on the element, which may cause some surprise if a user changes the method and the React component doesn't update. Let me try to explain this better:

  • For all props, we set up a setter for each property such that when you change the value on the prop, it updates the corresponding attribute value and runs update() on the React root if it's mounted. You can see this start at https://github.com/bitovi/react-to-web-component/blob/main/packages/core/src/core.ts#L175
  • So when a string or some converted string changes on the element's properties (or attributes) the value is updated in the props sent to the React component and the root's update() gets called. For functions, the same idea applies but it just looks at the new property name on the window and passes whatever it finds there as the value before updating the root.

Now what you're proposing here is a second property on the element, pointed to by the value of the first, that's going to hold a function. But what if that second property's value changes? Should a user expect that to cause a React root update and trigger effects/memos/etc. that depend on that function? I'd say probably! We didn't do this on the global for the function transformer because we don't want to be doing arbitrary prop definitions on the global object. But for the component element's prototype we have more leeway. What do you think?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants