Closed nacmartin closed 7 years ago
Nice work!
I think that since we are using a Symfony Request {...}
Implementing it like this is very convenient for the end user since he never has to think about it. But I was wondering, doesn't it limit non Symfony / Silex / RequestStack users from using the Renderer? I mean, I'm not sure how but currently the Renderer could be easily wrapper for use in Larvel right? Would that still be the case?
I'm not sure how to approach this, but wouldn't be an idea to use this provider as the default, but maybe also allow the user to register a custom provider, allowing a custom set of properties to be passed in? Not sure where to register this provider though.. it's not ideal to do it in the twig template.. but on registration of the bundle / provider is useless since no request info is there yet.. It's difficult.
Anyway, it's all good for me, I would personally be very happy with the Symfony request approach you've taken :)
However, I think that since we are using a Symfony Request it would make more sense to use more "symfonic" keys for the array. So, instead of search, queryString; instead of pathname, pathInfo. The same case with requestUri and baseUrl.
As far as I can tell the only variable we need to keep the same is serverSide
because ReactOnRails relies on it. That being said, you never know what kind of breaking changes they introduce in a minor release. They might just stat using pathname
all of a sudden... I'm fine with both.
Ok, thanks! I have implemented your suggestions, especially about the ContextProvider, that is now an interface.
I plan to merge this soon.
Nice work :)
Cheers!
As pointed out in #4 it would be nice to populate rails context with some context about the request. I have done a first try, mimicking the object that creates React on Rails. This is conservative. However, I think that since we are using a Symfony Request it would make more sense to use more "symfonic" keys for the array. So, instead of
search
,queryString
; instead ofpathname
,pathInfo
. The same case withrequestUri
andbaseUrl
.Does this make sense? Any other comments?