search

Limenius / ReactRenderer

Client and Server-side React rendering from PHP
MIT License
237 stars 37 forks source link

StaticRenderer in 2.3.0 #27

Closed AlexandreKilian closed 5 years ago

AlexandreKilian commented 5 years ago

In 2.3.0 (https://github.com/Limenius/ReactRenderer/commit/48b98fb1d6b5006bcc5a288b46e416b8ee206b14#diff-39b69414ad8ca37f14d248ccb3163359), StaticReactRenderer was introduced as a parameter in ReactRenderer… What does this change/do? We now have to pass an AbstractReactRenderer as well as a SaticreactRenderer (which implements AbstractReactRenderer), why not just keep it to one abstract renderer and keep the decision which one to use (Static, PhpExecJs, …) to whoever uses it?

nacmartin commented 5 years ago

You are right, I was having doubts with that PR for other reasons as well, and I ended up merging it. Let me find a compromise so the idea behind this feature can work without messing with what you comment.

karlmacklin commented 5 years ago

I'll hold of making an issue in Alexandres repo then :)

nacmartin commented 5 years ago

I have reverted this change in the 2.x branch:

2.3.1 has the same API as the previous releases (basically forget about 2.3.0).

3.0 has temporary this static renderer (basically a cache), but I don't like the API so much so it will change hopefully soon.

I will try to work in a 4.0 which will implement caching of components with some refactoring of the Twig extension, without creating a new renderer, but having instead the renderer that is being used wrapped in a cache manager so we can cache components. It should be basically an extra option to pass to the twig tag, and if it is not passed everything should work as it was working before.

Now that I am aware of packages that are using ReactRenderer but not ReactBundle I will try to be more careful with API breaks ;)

AlexandreKilian commented 5 years ago

Great to hear it, thx! I locked the version in my repo to the last working one, but I guess I can remove that now :) Yes, I love the bundle and wanted to have a way to use it with craft too As for caching, it might be a better option to cache the output of the renderer, that's what I am doing in my projects