[LiveComponent] Support for liveProp union type #3150
Conversation
carsonbot
added
Feature
Kocal
left a comment
Kocal
left a comment
There was a problem hiding this comment.
Hi and thanks for the contribution!
This new feature needs to be covered with some tests, and the CHANGELOG.md must be updated too.
Also, you only support the TypeInfo path and not the legacy PropertyInfo path, but I think that's fine.
carsonbot
added
Status: Needs Work
carsonbot
added
Status: Needs Review
src/LiveComponent/CHANGELOG.md
Outdated
|
|
||
| - Add support for `LiveProp` union types in Live Components. | ||
| `LiveComponentMetadataFactory` can now create `liveProp` metadata for union types. | ||
| Note: This feature only works when using a custom `hydrate` and `dehydrate` implementation. |
There was a problem hiding this comment.
Hey @kevinmade ! Could you add a test or two covering the cases "not supposed to work" (ie: when hydrate and/or dehydrate are missing ?)
There was a problem hiding this comment.
I thought a hydrator and dehydrator were required for my change, but this had something to do with my own use case, where the union had a type that wasn't a hydratable. I’ve removed the note from the changelog and added a test.
Kocal
left a comment
Kocal
left a comment
There was a problem hiding this comment.
Please add tests in LiveComponentHydratorTest::provideDehydrationHydrationTests data provider, so we can test the full dehydration/hydration process. Testing only the metadata factory is not enough.
| public function testObjectTypedLivePropCannotBeDehydrated() | ||
| { | ||
| $componentClass = new class { | ||
| #[LiveProp(writable: true)] | ||
| public object $object; | ||
| }; | ||
|
|
||
| $component = new $componentClass(); | ||
| $component->object = new class { | ||
| }; | ||
|
|
||
| $this->expectException(\LogicException::class); | ||
| $this->expectExceptionMessageMatches('/The "object" property on component ".*" is missing its property-type. Add the ".*" type so the object can be hydrated later./'); | ||
|
|
||
| $this->hydrator()->dehydrate($component, $this->createComponentAttributes(), $this->createLiveMetadata($component)); | ||
| } |
There was a problem hiding this comment.
To move in \Symfony\UX\LiveComponent\Tests\Integration\LiveComponentHydratorTest::provideDehydrationHydrationTests
| public function testLivePropUnionType() | ||
| { | ||
| /** @var LiveComponentMetadataFactory $metadataFactory */ | ||
| $metadataFactory = self::getContainer()->get('ux.live_component.metadata_factory'); | ||
|
|
||
| $class = new \ReflectionClass(WithUnionIntersectionType::class); | ||
| $propsMetadata = $metadataFactory->createPropMetadatas($class); | ||
|
|
||
| $propsMetadataByName = []; | ||
| foreach ($propsMetadata as $propMetadata) { | ||
| $propsMetadataByName[$propMetadata->getName()] = $propMetadata; | ||
| } | ||
|
|
||
| $this->assertEquals('mixed', (string) $propsMetadataByName['unionProp']->getType()); | ||
| $this->assertEquals('mixed', (string) $propsMetadataByName['intersectionProp']->getType()); | ||
| } |
There was a problem hiding this comment.
I don't think it makes sense to have this test
| if ($reflectionType instanceof \ReflectionUnionType || $reflectionType instanceof \ReflectionIntersectionType) { | ||
| return new LivePropMetadata($property->getName(), $liveProp, Type::mixed()); | ||
| } |
There was a problem hiding this comment.
Why do we use Type::mixed() here, instead of $this->typeResolver->resolve($property) below?
carsonbot
added
Status: Needs Work
4 participants
This PR adds support for liveProp union types in Live Components. Previously, using a union type would throw an exception. With this change
LiveComponentMetadataFactorywill be able to create liveProp metadata for union types, but will only work in combination with a custom hydrate and dehydrate implementation.