React Renderer: refactor the code, clean it up and make it more generic
This series of commits is functionally nearly identical to the current code (save for a bug fix to the sandbox that nobody uses yet). However it improves the code in these ways:
- It's smaller and simpler now.
- The
utilssub-package contains classes that could be integrated into Micronaut Core. They aren't React, Graal or Views specific. - The
trufflesub-package contains classes that could be re-used for a more generic Truffle integration. - More objects are overridable via factories, and beans are now properly namespaced using qualifiers.
- The docs for how to use hot-reload have been improved.
I hope that in future the generic code can find a new home in other modules.
Can this be merged?
@dstepanov PTAL
I didn't check the PR more closely before, so now I have a better look. Instead of doing in-code comments, I will try to summarise it here:
-
@Named(REACT_QUALIFIER)- it would be better to have a custom qualifier annotation - Not sure why do you need to create
NamedSourceQualifier -
Context.newBuilder()options might be better to be provided as a configuration (if it makes sense)Context.Buildermight be represented as@ConfigurationBuilder. The same might be applicable forEngine.newBuilder - Having some synchronized state in a factory is very strange and should be grouped with the actual state
- Essentially, it looks like the bean pool should be created from some configuration state that will hold all the sources, and if the event changes the configuration, the pool should be purged, and the configuration should be recreated with the new values. This logic of the clearing the pool shouldn't be part of
ReactViewsRenderer
IMHO, the best would be to have BeanPool (as an interface, preferably) and a BeanPoolInstanceProvider with a callback that the pool will use to purge the bean pool.
Sometimes, we would make builders as beans which will allow users to alter the configuration
Being able to control the context via config etc is a good idea, but it feels like a separate PR. I'm trying to keep this one a functionality no-op, just a refactoring. It's already got quite large.
The BeanPool is purged when source files on disk change not when configuration changes, so I didn't fully understand that suggestion, apologies. The existing refresh mechanisms don't seem applicable here.
When you say group the synchronized methods with the state, could you elaborate what you have in mind? The style linter doesn't allow fields to be moved next to the methods that manipulate them. Do you mean move the file change listener into the factory? The issue is that the things in the BeanPool are vended by the Factory, so the factory has to be told to re-create the beans in the same way as initially, but it's the owner of the pool that has to clear them.
I’m talking about the design. The configuration should be affected by the listener, and the bean pool should be affected by the configuration. Now it’s a bit messy.
Which configuration do you mean? The bean pool is only affected by a change in the checksum of the files on disk, which isn't reflected in any configuration bean. This is for hot reload scenarios.
I call all the files that affect the context instance as configuration.
@dstepanov How does it look now?
@mikehearn Please check https://github.com/mikehearn/micronaut-views/pull/1
@mikehearn Can you please resolve conflicts and make sure the build is passing?
Looks like the Java 21 build hit a Gradle bug?!