micronaut-views icon indicating copy to clipboard operation
micronaut-views copied to clipboard

React Renderer: refactor the code, clean it up and make it more generic

Open mikehearn opened this issue 1 year ago • 1 comments

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:

  1. It's smaller and simpler now.
  2. The utils sub-package contains classes that could be integrated into Micronaut Core. They aren't React, Graal or Views specific.
  3. The truffle sub-package contains classes that could be re-used for a more generic Truffle integration.
  4. More objects are overridable via factories, and beans are now properly namespaced using qualifiers.
  5. 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.

mikehearn avatar Aug 16 '24 12:08 mikehearn

Can this be merged?

mikehearn avatar Sep 25 '24 18:09 mikehearn

@dstepanov PTAL

mikehearn avatar Oct 07 '24 12:10 mikehearn

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.Builder might be represented as @ConfigurationBuilder. The same might be applicable for Engine.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.

dstepanov avatar Oct 07 '24 13:10 dstepanov

Sometimes, we would make builders as beans which will allow users to alter the configuration

dstepanov avatar Oct 07 '24 14:10 dstepanov

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.

mikehearn avatar Oct 08 '24 14:10 mikehearn

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.

dstepanov avatar Oct 08 '24 15:10 dstepanov

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.

mikehearn avatar Oct 08 '24 18:10 mikehearn

I call all the files that affect the context instance as configuration.

dstepanov avatar Oct 09 '24 15:10 dstepanov

@dstepanov How does it look now?

mikehearn avatar Oct 14 '24 12:10 mikehearn

@mikehearn Please check https://github.com/mikehearn/micronaut-views/pull/1

dstepanov avatar Oct 15 '24 08:10 dstepanov

@mikehearn Can you please resolve conflicts and make sure the build is passing?

dstepanov avatar Oct 15 '24 09:10 dstepanov

Looks like the Java 21 build hit a Gradle bug?!

mikehearn avatar Oct 16 '24 15:10 mikehearn