Make it possible to change the react server's url on the fly

[jphalip]

Currently the node server's URL is set at module loading time: https://github.com/markfinger/python-react/blob/master/react/render_server.py#L76

It'd be great to make it so that the URL can be changed dynamically, so it can then be controlled during the running of a test suite, for example.

If you agree with the idea then I'm happy to work on a patch for this. Thanks!

[markfinger]

Yeah, sounds great @jphalip

[jphalip]

So I realized that this could in fact already be achieved by resetting the render_server's url attribute dynamically. This is what I ended up doing:

from subprocess import Popen, PIPE
from django.test.runner import DiscoverRunner
from django.conf import settings
from react.render_server import render_server

TEST_REACT_SERVER_HOST = getattr(settings, 'TEST_REACT_SERVER_HOST', '127.0.0.1')
TEST_REACT_SERVER_PORT = getattr(settings, 'TEST_REACT_SERVER_PORT', 9008)

class CustomTestRunner(DiscoverRunner):
    """
    Same as the default Django test runner, except it also runs our node
    server as a subprocess so we can render React components.
    """

    def setup_test_environment(self, **kwargs):
        # Start the test node server
        self.node_server = Popen(
            [
                'node',
                'react-server.js',
                '--host=%s' % TEST_REACT_SERVER_HOST,
                '--port=%s' % TEST_REACT_SERVER_PORT
            ],
            stdout=PIPE
        )
        # Wait until the server is ready before proceeding
        self.node_server.stdout.readline()
        # Point the renderer to our new test server
        render_server.url = 'http://%s:%s' % (
            TEST_REACT_SERVER_HOST, TEST_REACT_SERVER_PORT)
        super(CustomTestRunner, self).setup_test_environment(**kwargs)

    def teardown_test_environment(self, **kwargs):
        # Kill the node server
        self.node_server.terminate()
        super(CustomTestRunner, self).teardown_test_environment(**kwargs)

I'm not sure if that could be done differently in a way that wouldn't be backwards incompatible. Do you have any opinions? If not then I'm ok to just close this ticket and move forward with the solution above :)

[markfinger]

Hmm, the pattern I've tended to use elsewhere has been to provide a final arg on functions that allows you to override any settings. This enables you to use globals as sane defaults, and also easily override them for high-level integration tests - you can also mock or rewrite stuff, but I generally prefer to avoid mutating things.

Something like...

render_component(
    # ..., 
    setting_overrides={
        'RENDER_URL': '...'
    },
)

It'd probably be worth changing the signatures to something like

def render_component(path, props=None, to_static_markup=False, renderer=render_server, setting_overrides=None):
    # ...

class RenderServer(object):
    # ...
    def render(self, path, props=None, to_static_markup=False, setting_overrides=None):
        # ...

You end up with a bit of a dance to check the overrides and fallback to the globals, but that's easily abstracted into a function.

---

I'm happy to accept a PR, if you want to make any changes. Either way, I'll leave this open until it's resolved.

[jphalip]

So what I had in mind was to source the URL from inside the render() method instead of init():

--- a/./render_server.py
+++ b/./render_server.py
@@ -20,8 +20,6 @@ class RenderedComponent(object):

 class RenderServer(object):
-    def __init__(self, url):
-        self.url = url

     def render(self, path, props=None, to_static_markup=False):
         if props is not None:
@@ -40,19 +38,21 @@ class RenderServer(object):
         serialized_options = json.dumps(options)
         options_hash = hashlib.sha1(serialized_options.encode('utf-8')).hexdigest()

+        url = conf.settings.RENDER_URL
+
         try:
             res = requests.post(
-                self.url,
+                url,
                 data=serialized_options,
                 headers={'content-type': 'application/json'},
                 params={'hash': options_hash}
             )
         except requests.ConnectionError:
-            raise RenderServerError('Could not connect to render server at {}'.format(self.url))
+            raise RenderServerError('Could not connect to render server at {}'.format(url))

         if res.status_code != 200:
             raise RenderServerError(
-                'Unexpected response from render server at {} - {}: {}'.format(self.url, res.status_code, res.text)
+                'Unexpected response from render server at {} - {}: {}'.format(url, res.status_code, res.text)
             )

         obj = res.json()
@@ -73,4 +73,5 @@ class RenderServer(object):
         return RenderedComponent(markup, serialized_props)

-render_server = RenderServer(conf.settings.RENDER_URL)
+render_server = RenderServer()

That way you could do integration testing like this:

from django.test import TestCase, override_settings

class MyTests(TestCase):

    def test_something(self):
        with self.settings(REACT={'RENDER': True, 'RENDER_URL': '...'}):
            # Do something

    @override_settings(REACT={'RENDER': True, 'RENDER_URL': '...'})
    def test_anotherthing(self):
        # Do another thing

This would feel more like the traditional way of enforcing global settings during the tests, but it'd be a backwards incompatible change. That said, the above doesn't seem to work currently anyways, i.e. override_settings() and self.settings() actually don't change the react app's settings. I suppose this might be due to how the ReactConfig class is managed and it seems that the settings are locked down and can't be overridden, but I'm not sure.

What do you think?

[markfinger]

Yeah, the frozen settings instance is a quirk of supporting both django and non-django systems.

There are _unlock and configure methods on the settings object, but I tend to use mock to replace the settings variable in the conf object.

Are there any events emitted by django when test cases swap out settings? If so, that'd reduce the boilerplate and allow us to rely on django's settings as a single source of truth.

[markfinger]

Easiest path might be to always proxy the setting lookups to django.conf.settings.REACT.*, falling back to the default.

So closing this would need:

• ensure that the render url lookup happens during render calls, rather than init
• settings should always reflect the django's settings, even if django's have changed since init

[markfinger]

@jphalip when it's running under django, it should proxy to django.conf.settings now.

Do you mind giving it a whirl and seeing if it works for you?

[jphalip]

@markfinger Sorry for the delay. I just checked and it does work indeed now with your latest updates. Thank you!

[markfinger]

Cool, thanks for checking!