Fix instantiating LiveComponentMetadata multiple times #1251
Conversation
| @@ -33,12 +33,17 @@ public function __construct( | |||
|
|
|||
| public function getMetadata(string $name): LiveComponentMetadata | |||
| { | |||
| static $liveComponentMetadata; | |||
There was a problem hiding this comment.
Maybe a private typed property ?
There was a problem hiding this comment.
I could change it to
/** @var LiveComponentMetadata[] */
private static array $liveComponentMetadata = [];
...
public function getMetadata(string $name): LiveComponentMetadata
{
if (isset(self::$liveComponentMetadata[$name])) {
return self::$liveComponentMetadata[$name];
}
$componentMetadata = $this->componentFactory->metadataFor($name);
$reflectionClass = new \ReflectionClass($componentMetadata->getClass());
$livePropsMetadata = $this->createPropMetadatas($reflectionClass);
return self::$liveComponentMetadata[$name] = new LiveComponentMetadata($componentMetadata, $livePropsMetadata);
}
There was a problem hiding this comment.
Yup - do this. But don't make the property static. In practice, there will only be one LiveComponentMetadataFactory service, so static vs non-static won't make any real difference. But we should not enforce that there can only be one instance of this object by using static.
| $componentMetadata = $this->componentFactory->metadataFor($name); | ||
|
|
||
| $reflectionClass = new \ReflectionClass($componentMetadata->getClass()); | ||
| $livePropsMetadata = $this->createPropMetadatas($reflectionClass); | ||
|
|
||
| return new LiveComponentMetadata($componentMetadata, $livePropsMetadata); | ||
| return $liveComponentMetadata[$name] = new LiveComponentMetadata($componentMetadata, $livePropsMetadata); |
There was a problem hiding this comment.
Are we sure we want to share a single metadata instance for all the components of the same name ?
There was a problem hiding this comment.
The ComponentFactory metadataFor method also returns ComponentMetadata with configurations based on name, so it won't be a problem.
There was a problem hiding this comment.
But is the instance modified elsewhere in the code ?
There was a problem hiding this comment.
The metadata DTO LiveComponentMetadata that will be stored only has private propeties and getters. All data within the DTO is static and related to property attribute LiveProp and static component configurations.
There was a problem hiding this comment.
Yup, this looks good to me too. LiveComponentMetadata is meant to be the metadata for the component with the name $name. If we ever modify any of that metadata on component-by-component basis, that should be considered a bug (and I don't see any of that at the moment).
| @@ -33,12 +33,17 @@ public function __construct( | |||
|
|
|||
| public function getMetadata(string $name): LiveComponentMetadata | |||
| { | |||
| static $liveComponentMetadata; | |||
There was a problem hiding this comment.
Yup - do this. But don't make the property static. In practice, there will only be one LiveComponentMetadataFactory service, so static vs non-static won't make any real difference. But we should not enforce that there can only be one instance of this object by using static.
| $componentMetadata = $this->componentFactory->metadataFor($name); | ||
|
|
||
| $reflectionClass = new \ReflectionClass($componentMetadata->getClass()); | ||
| $livePropsMetadata = $this->createPropMetadatas($reflectionClass); | ||
|
|
||
| return new LiveComponentMetadata($componentMetadata, $livePropsMetadata); | ||
| return $liveComponentMetadata[$name] = new LiveComponentMetadata($componentMetadata, $livePropsMetadata); |
There was a problem hiding this comment.
Yup, this looks good to me too. LiveComponentMetadata is meant to be the metadata for the component with the name $name. If we ever modify any of that metadata on component-by-component basis, that should be considered a bug (and I don't see any of that at the moment).
| @@ -25,6 +25,9 @@ | |||
| */ | |||
| class LiveComponentMetadataFactory | |||
| { | |||
| /** @var LiveComponentMetadata[] */ | |||
| private array $liveComponentMetadata = []; | |||
|
|
|||
There was a problem hiding this comment.
This empty line has some extra whitespace (you can see it in the php-cs-fixer job)
There was a problem hiding this comment.
This is what happens when using committing via github.com 😄
weaverryan
force-pushed
the
fix-metadata-factory
branch
from
2603216 to
7e03119
Compare
|
Fantastic! Thanks @TheDutchScorpion! |
|
Maybe this should implement Symfony's ResetInterface in case of long-running webserver? |
|
I'm not sure. Are Doctrine or validation metadata, for example, reset in this way? Or, perhaps those use proper caching vs storing on a property. So, you might have a point |
|
Should I open a PR for this or do you make this change? @weaverryan |
|
Thanks @TheDutchScorpion 👍 |
I mean that if we don't reset it could eat more memory than it should to without releasing the cache, so I think it's fine to have in memory cache for fetching metadata of same component many times, but sometimes it needs to be cleared up :) |
…hScorpion) This PR was squashed before being merged into the 2.x branch. Discussion ---------- Add ResetInterface to LiveComponentMetadataFactory | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | Issues | N/A | License | MIT This PR is a follow-up to the already merged PR #1251 Commits ------- 8d3d328 Add ResetInterface to LiveComponentMetadataFactory
This pull request fixes an issue with instantiating LiveComponentMetadata multiple times, when loading the same component multiple times.