[labs/react] Fix for #2799: add support for properties with custom accessors

[lideen]

Hi! All new to this repo but tried to fix a bug that I found #2799

Any input appreciated 🎉

[changeset-bot]

🦋 Changeset detected

Latest commit: 5c4eb26

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@lit-labs/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

For more information, open the CLA check for this pull request.

[justinfagnani]

@lideen thanks for identifying this and submitting a fix and tests!

I made some comments on the implementation in the interest of speed here. I think the implementation can be collapsed into one getInstancePropertiesOf() function that recursively calls itself on a class's superclass, merges the superclass's properties into its own, then caches and returns the result. This will avoid materializing the prototype chain and processing prototypes multiple times. It should minimize memory allocations too.

I would like to also avoid materializing the property descriptors if possible. I think the core issue here is that accessors are not enumerable and for...in doesn't include non-enumerable properties. Object.getOwnPropertyNames() does return non-enumerable names (Reflect.ownKeys()) does too, but it's not supported on IE11 and we don't need Symbol-keyed properties in JSX). So I hope we can use that. It would include methods, but that might be ok.

[lideen]

@justinfagnani Something like this? 1531750

[justinfagnani]

@lideen that what I was thinking at the time (though with memoizing the function).

But in the meantime I chatted with @josepharhar about it, and I think that we should match the Preact and upcoming React support for properties as much as possible. This would mean not pre-calculating the instance keys at all, but just using an in check on each given prop key. See https://github.com/facebook/react/blob/2c8a1452b82b9ec5ebfa3f370b31fda19610ae92/packages/react-dom/src/client/DOMPropertyOperations.js#L187-L194

[lideen]

@justinfagnani I gave it a shot 79e6a4f. I removed the warning about reserved props as I don't think it fits well now when the check is done on-demand 🤔 . Let me know if I should re-add it.

[lideen]

@justinfagnani sorry to ping you, but is there anything I can do to move this along?

[justinfagnani]

@lideen So sorry I missed that ping!

I'm re-reviewing now. My main concern is how we verify that the behavior matches React 19, but we can address that over time.

[justinfagnani]

We still need to verify this logic (specifically excluding k in HTMLElement.prototype) with @josepharhar to see if it matches React 19. I'm good with going in as-is now though.

[justinfagnani]

I think we should keep this warning. We can move this into a dev build in the future.

[justinfagnani]

@lideen this has increased in urgency for us, so I'm going to try to see if I can get this in asap. Did you enable changes to your PR branch? https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Edit: looks like I can push to your fork. Great!

[justinfagnani]

Thanks @lideen !

I'll follow up on the comments I left with issues.