hfour / h4bff

H4's backend & frontend framework :cupid::wrench: https://hfour.github.io/h4bff/
MIT License
7 stars 2 forks source link

Pluggable router #12

Closed EmilH4 closed 5 years ago

EmilH4 commented 5 years ago

Introduces a pluggable mobx router.

This should be able to fully substitute the router from react-router, including the component.

For more information, see the wiki page: https://github.com/hfour/h4bff/wiki/Router-%5Bfrontend%5D

EmilH4 commented 5 years ago

Tests. Lets try to set a boundary where the Mobx router ends and React rendering begins -- this will allow us to naturally test router behavior. Ideally the tests will take the form of: 1. I define some routes 2. I visit this URL 3. Is the expected page (function) rendered (called)?

I'd not separate MainRouter and Router. Initializing new Router() will create an out-of-band instance which isn't mockable, discoverable, hence unpluggable. If we need nesting, we can always create child apps using bff core.

After tests, we can talk about renaming stuff and shuffling code a bit, but not before that.

Good job overall!!

The tests are improved. About the usage (and exposure) of Router:

  1. I think that they should be separate entities, even if Router isnt exposed - there is some difference in what they do, and its nice to separate it.
  2. As to whether Router should be exposed:
    • On a project, we can agree that we shouldnt use it, and that solves the problem. If some other project wishes to use the "pluggable" way, let them, no need to constrain at this point
    • There are some problems, like rendering 2 different routes at the same time, which has to be done only with including 2 routers (with the current setup).

Example for the last point:

Lets take a slack-like design for example, where the url would be like /:channel/:thread, and one part of the screen should react on change of the channel id, while another on the thread id. We could create 2 separate routers, each with one route:

let channelRouter = new Router(app);
channelRouter.addRoute('/:channel/*', () => <ChannelComponent/>

let threadRouter = new Router(app);
threadRouter.addRoute('/:something/:thread', () => <ThreadComponent/>

let finalJsx = <Container> 
                  <channelRouter.RenderInstance/>
                  <threadRouter.RenderInstance/>
               <Container/>

Having this in mind, I think that this PR is ready to be merged

wh4everest commented 5 years ago

On a project, we can agree that we shouldnt use it, and that solves the problem. If some other project wishes to use the "pluggable" way, let them, no need to constrain at this point

Since this code is in the h4bff repo, we shouldn't sweat over cases where people want to use stuff out-of-band.

There are some problems, like rendering 2 different routes at the same time, which has to be done only with including 2 routers (with the current setup).

That's not completely true. You could create 2 sub-apps (singleton containers) and initialize the routers in one of each. If this mechanism is not sufficient, we can talk how to improve it / replace it, but again, initializing BFF classes with new should be strongly discouraged.