Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

mikehearn
Copy link
Contributor

@mikehearn mikehearn commented Aug 16, 2024

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.

@sdelamo sdelamo changed the base branch from 5.4.x to 5.5.x August 21, 2024 07:19
@@ -13,12 +13,10 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.micronaut.views.react;
package io.micronaut.views.react.util;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this class should be in micronaut-logging

*/
@Internal
public class BeanPool<T> {
// TODO: Use @Scheduled to occasionally clear out beans that weren't accessed for a while to recover from traffic spikes.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

address this TODO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we do it in a separate PR? This one is trying to be a functionality no-op as far as possible.

@mikehearn
Copy link
Contributor Author

Can this be merged?

@mikehearn
Copy link
Contributor Author

@dstepanov PTAL

@dstepanov
Copy link

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
Copy link

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

@mikehearn
Copy link
Contributor Author

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.

@dstepanov
Copy link

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.

@mikehearn
Copy link
Contributor Author

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.

@dstepanov
Copy link

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

@mikehearn
Copy link
Contributor Author

@dstepanov How does it look now?

@dstepanov
Copy link

@mikehearn Please check mikehearn#1

@dstepanov
Copy link

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

@sdelamo sdelamo changed the base branch from 5.5.x to 5.6.x October 15, 2024 17:12
@mikehearn
Copy link
Contributor Author

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

This version fixes a JVM crash that occurs with the latest GraalVM, and removes the need for the prior test disables related to sandboxing.

The JSON object ordering isn't stable across versions so fix the assumption that it is.
This is a new generic utility useful for not only other Truffle languages, but Micronaut in general.

Improved the documentation, efficiency and usability of the class along the way.
This class is generally useful and could be a part of Micronaut (or really, SLF4J itself).
mikehearn and others added 23 commits October 30, 2024 16:02
This class is generally useful and could be a part of Micronaut (or really, SLF4J itself).
List/map access wasn't previously allowed, so in sandbox mode lists and maps wouldn't be exposed to JS and would appear empty.
1. ContextPoolManager now encapsulates source loading and reloading.
2. Encapsulated @nAmed("react") behind a custom annotation.
3. The named qualifier resolver is no longer needed.
@sdelamo sdelamo merged commit 65d4d19 into micronaut-projects:5.6.x Nov 7, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants