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

A fractal-component implementation which respects the open/closed principle #46

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ghola
Copy link

@ghola ghola commented Oct 19, 2018

Changes to the fractal-lcomponent implementation so it respects the open/closed principle. More details about them are present in #45.

@t83714 It's not functional, but you can clearly see the intent. I have encountered several issues:

  1. I thought I could do without ActionForwarders, but aside from publishing the actions in the global namespace, there seems to be no way a component can listen to them if they are not forwarded. Maybe some of the forwarder functionality built into the component would help (something like the allowedIncomingMulticastActionTypes array which would allow you also specify the namespace you want to listen to).
  2. Adding two forwarders for the same namespace but for different actions seems to forward the same action twice. An example of this is randomGifActionTypes.TOGGLED_ON which gets dispatched once by the ToggleButton component, and then an additional 2 times by the forwarder (when it should be just once by the forwarder).
  3. Forwarding randomGifActionTypes.NEW_GIF actions with relativeDispatchPath="../App/*" results in an error:
TypeError: taker[_redux_saga_symbols__WEBPACK_IMPORTED_MODULE_2__.MATCH] is not a function
    at redux-saga-core.esm.js:309
    at Array.forEach (<anonymous>)
    at put (redux-saga-core.esm.js:308)
    at redux-saga-core.esm.js:937
    at exec (redux-saga-core.esm.js:31)
    at flush (redux-saga-core.esm.js:74)
    at asap (redux-saga-core.esm.js:46)
    at runPutEffect (redux-saga-core.esm.js:933)
    at runEffect (redux-saga-core.esm.js:793)
    at digestEffect (redux-saga-core.esm.js:870) ""
log @ chunk-70f0280b.js:199
logError @ redux-saga-core.esm.js:1299
end @ redux-saga-core.esm.js:757
abort @ redux-saga-core.esm.js:497
task$$1.cont @ redux-saga-core.esm.js:512
end @ redux-saga-core.esm.js:766
abort @ redux-saga-core.esm.js:497
task$$1.cont @ redux-saga-core.esm.js:512
end @ redux-saga-core.esm.js:766
abort @ redux-saga-core.esm.js:497
task$$1.cont @ redux-saga-core.esm.js:512
end @ redux-saga-core.esm.js:766
abort @ redux-saga-core.esm.js:497
task$$1.cont @ redux-saga-core.esm.js:512
next @ redux-saga-core.esm.js:731
currCb @ redux-saga-core.esm.js:840
(anonymous) @ redux-saga-core.esm.js:939
exec @ redux-saga-core.esm.js:31
flush @ redux-saga-core.esm.js:74
asap @ redux-saga-core.esm.js:46
runPutEffect @ redux-saga-core.esm.js:933
runEffect @ redux-saga-core.esm.js:793
digestEffect @ redux-saga-core.esm.js:870
next @ redux-saga-core.esm.js:717
currCb @ redux-saga-core.esm.js:840
Promise.then (async)
resolvePromise @ redux-saga-core.esm.js:884
runCallEffect @ redux-saga-core.esm.js:964
runEffect @ redux-saga-core.esm.js:793
digestEffect @ redux-saga-core.esm.js:870
next @ redux-saga-core.esm.js:717
currCb @ redux-saga-core.esm.js:840
(anonymous) @ redux-saga-core.esm.js:946
exec @ redux-saga-core.esm.js:31
flush @ redux-saga-core.esm.js:74
asap @ redux-saga-core.esm.js:46
chan.put @ redux-saga-core.esm.js:343
(anonymous) @ redux-saga-core.esm.js:1393
dispatch @ VM11265:1
dispatch @ fractal-component.esm.js:845
onClick @ index.js:88
callCallback @ react-dom.development.js:145
invokeGuardedCallbackDev @ react-dom.development.js:195
invokeGuardedCallback @ react-dom.development.js:248
invokeGuardedCallbackAndCatchFirstError @ react-dom.development.js:262
executeDispatch @ react-dom.development.js:593
executeDispatchesInOrder @ react-dom.development.js:615
executeDispatchesAndRelease @ react-dom.development.js:713
executeDispatchesAndReleaseTopLevel @ react-dom.development.js:724
forEachAccumulated @ react-dom.development.js:694
runEventsInBatch @ react-dom.development.js:855
runExtractedEventsInBatch @ react-dom.development.js:864
handleTopLevel @ react-dom.development.js:4857
batchedUpdates$1 @ react-dom.development.js:17498
batchedUpdates @ react-dom.development.js:2189
dispatchEvent @ react-dom.development.js:4936
interactiveUpdates$1 @ react-dom.development.js:17553
interactiveUpdates @ react-dom.development.js:2208
dispatchInteractiveEvent @ react-dom.development.js:4913
14:36:28.075 chunk-70f0280b.js:199 The above error occurred in task bound _callee
    created by takeEvery(, bound _callee)
    created by forwardNamespacedAction
    created by bound hostSaga
Tasks cancelled due to error:
takeEvery(, bound _callee)
startCommandChan 

Anyway, you can fix it if you think it's worth it. My goal was only to exemplify the ideas discussed in #45, so I will not continue developing this example.

I was mentioning a change in business rules so we can see how solutions adapt to that. Here are some possible rule changes:

  1. You need 2 ToggleButtons and only when they are both active will the counter increment by 2 as per the original business rule. So instead of 1 Button now you have 2 and the condition that they are both active.
  2. We keep a single ToggleButton, same logic as in the original business rule but you add the fact that you need to increase Counter by 3 (instead of 2) if the ToggleButton has been clicked more than 5 times and the button is Active.

Assume that your components are published as NPM modules. In your implementation you need to make changes to them in order to implement the above business rules. In my implementation you don't, you only need to make changes to the App component which is not published to NPM.

@t83714
Copy link
Contributor

t83714 commented Oct 19, 2018

Hi @ghola,
Thanks a lot for your code~ It definitely helps to understand your idea better 👍

1> Regarding the TypeError error that you encountered, I think the problem is around the following lines (in your App saga):

     yield effects.takeEvery(
        ToggleButtonActionTypes.TOGGLED_OFF,
        function*() {
            console.log('off');
            this.isToggledOn = false;
        }.bind(this)
    );

ToggleButtonActionTypes.TOGGLED_OFF is undefined as TOGGLED_OFF wasn't exported from ToggleButton/index.js

Having said that, either fractal-component or redux-saga should give user a more clear error message. I've updated fractal-component for this.

2> While looking into your issue, I discovered a compatible issue with redux-saga v1.0.0-beta.3. I updated fractal-component for that as well.

You probably won't be impacted by this issue as redux-saga v1.0.0-beta.3 was just out two days ago.

3> You don't need to rely on ActionForwarders to dispatch events and components don't need to explicitly listen to any actions sources as actions deliver scope has been defined by the two piece information when you dispatch actions:

  • relativeDispatchPath
  • Is multicast action (relativeDispatchPath ends with *) or not

A multicast action won't be delivered to every components in your app (as it's not efficient and causing a lot of problems). Instead, multicast actions are passed down along the namespace tree branch starting from the namespace node determined by the relativeDispatchPath--- more details can be found from: https://github.com/t83714/fractal-component/blob/master/docs/Introduction/BeginnerTutorial/RandomGif/Namespace.md

4>

Adding two forwarders for the same namespace but for different actions seems to forward the same action twice. An example of this is randomGifActionTypes.TOGGLED_ON which gets dispatched once by the ToggleButton component, and then an additional 2 times by the forwarder (when it should be just once by the forwarder).

It seems randomGifActionTypes.TOGGLED_ON is undefined as well. TOGGLED_ON is not exported from randomGif/index.js.

5>

I was mentioning a change in business rules so we can see how solutions adapt to that. Here are some possible rule changes:

  1. You need 2 ToggleButtons and only when they are both active will the counter increment by 2 as per the original business rule. So instead of 1 Button now you have 2 and the condition that they are both active.
  2. We keep a single ToggleButton, same logic as in the original business rule but you add the fact that you need to increase Counter by 3 (instead of 2) if the ToggleButton has been clicked more than 5 times and the button is Active.

I like the two scenarios you described here. It does provide more useful sample cases for more discussion around this area 😄

Regarding your solution, it seems it move some logic out to App level. I think we probably look for a solution of encapsulating logic into components. You can argue that having a flat stucture app (i.e. lift up all state & logic to top level & make all component are presental only) can adapt any changes in any scenarios. However, we also end up having no reusable component.

To be honest, I think for component encapsulation, most important thing is to try making no assumption of outside world.

It's probably OK that an encapsulated component can't meet everyone's need (or any changes) as it's supposed to be reused for a certain purpose. If a user finds a component can't meet his requirement, he probably can ask the component author to update the component to accomodate the new logic.
If it's too hard, we probbaly need a new component --- at end of the day, we probably can't expect one component solve all problems. On the other hand, a solution can cope with any logics in this context probably would be an app with no encapsulated code.

Having said that, I am still quite interested in the two scenarios you've described above. Are you able to demostrate a solution that:

  • can encapsulate all logic into components
  • can adopt all three scenarios (including the original logic)
    e.g. the solution should not have the following logic at app level
if (this.count > 10 && this.isToggledOn) {
    yield effects.put(CounterActions.increaseCount());
}

P.S. If possible, could you please create your PR to a different directory? e.g. fractal-component-enhanced-example
This way people who're interested in this topic can look at both solutions and probably can benefit more.
Cheers~

@ghola
Copy link
Author

ghola commented Oct 19, 2018

Hey @t83714, thank you for the hints on the errors I encountered. Not using TypeScript sure makes one's life harder.

3> I understand how actions get published down the namespace tree (it's an entire discussion whether this is a harmful or a beneficial limitation). I also agree with you should throw the action just out of the your component (as per your comments in the code). That's why I kept the original relativeDispatchPath (yield effects.put(actions.newGif(), "../../../*"); for all components and that's why I said that I have to use the ActionForwarder because otherwise there was no way I could hook into those actions due to the level they were thrown at.

Given the way fractal-components works, I believe it may be even dangerous to throw the action higher in the namespace tree, mostly because your component has no idea how deep it is, so it can't know how many slashes to add.

One alternative would be just throwing it in the global namespace, but I think that should be avoided as well. Thinking in terms of RabbitMQ topics for a bit here, you can publish messages on a distinct topic/exchange and only interested parties will subscribe and receive them. Throwing them in the global topic is just pollution, especially when you might have difficulties distinguishing between message types (I know Symbol ensures uniqueness, but when you look in your redux dev tools at the actions, you'll still be confused).

In the end, that's why I suggested that a component should be able to subscribe to messages from a different namespace without resorting to using the ActionForwarder.

5>

Regarding your solution, it seems it move some logic out to App level. I think we probably look for a solution of encapsulating logic into components. You can argue that having a flat stucture app (i.e. lift up all state & logic to top level & make all component are presental only) can adapt any changes in any scenarios. However, we also end up having no reusable component.

Imagine you have a chat application which is composed of a left side panel which lists the users and a right panel which displays the conversation with the currently selected user (very similar to Whatsapp for desktop). When you receive a new message for a user which is not the one you're currently chatting with, you need to display the number of unread messages next to the user's icon. Let's also assume that it's the right panel doing the checking/loading of all the messages and can display the current conversation for the user being passed via props. The right panel component needs to notify the left side panel of the messages as they arrive, so the side panel can display those bubbles next to the user's icon. Also, it needs to notify the side panel when the messages get read (maybe there are many messages and they get read as the right panel gets scrolled). These two can be pretty independent components, siblings at application level, but they need to communicate. In this contrived example the communication is one way, but it could be two way as well. You'd still need some glue code and some state at application level, but doing that would not produce a flat structure app.

The two components representing the left and side panel would not be dumb at all, but they need to publish events to the outside world so others get a chance to react to them. It doesn't mean you lose encapsulation. It's like publishing events at the border of a domain context in DDD.

I cannot demonstrate a solution that encapsulates ALL logic into components. The "App" component is a forced one and is only there because it reduces friction with the framework (fractal-components). I don't believe that such a solution can exist, afterall that's what I'm militating against. I don't think that should be a goal worth pursuing. We all like symmetry and uniformity, but it's impractical to build your application with a single pattern. There is no pattern to rule them all.

I will make the PR to a different directory as you asked.

@ghola ghola force-pushed the better-open-closed-principle branch from 04695fb to 89576e9 Compare October 19, 2018 19:59
@ghola
Copy link
Author

ghola commented Oct 19, 2018

@t83714 Made it fully functional and made the push to the fractal-component-with-app-state directory.

@ghola ghola changed the title WIP: An implementation which respects the open/closed principle An implementation which respects the open/closed principle Oct 19, 2018
@ghola ghola changed the title An implementation which respects the open/closed principle A fractal-component implementation which respects the open/closed principle Oct 19, 2018
@ghola
Copy link
Author

ghola commented Oct 19, 2018

@t83714 On more thing regarding raising the state up to App level. That's not really necessary. The business logic here is only concerned with how the Counter component performs. This means we could make a CounterWrapper HOC which does all that forwarding/listening to events from other components. Whether such a component would be publishable as an NPM module, is debatable. I would say it's not because it's implementing very specific business rules.

This argument is just to counter the uneasiness you may have from having the state lifted all the way up to App level, if any.

@t83714
Copy link
Contributor

t83714 commented Oct 21, 2018

Hi @ghola,
Thanks for your update & sorry for my late reply 😭
Your code now runs smoothly~ Good job on that~ 👍
1> Regarding your concerns of dispatch / forward actions, I think in fractal-component you usually can control action dispatch by the following ways:

  • namespacePrefix: components share the same namespacePrefix will receive actions thrown out (from components) when actions are passing down the Namespace.
    e.g. For your App component, as you simply want to handle all actions in App component saga, you can simply set all child components' namespacePrefix to this.componentManager.fullPath(see doc here) to receive all actions and remove all actionForwarders. e.g. your App view could be something like:
           <div>
                <div className={classes.table}>
                    <div className={classes.cell}>
                        <RandomGif namespacePrefix={this.componentManager.fullPath} />
                    </div>
                    <div className={classes.cell}>
                        <Counter namespacePrefix={this.componentManager.fullPath}  />
                    </div>
                </div>
                <div className={classes.table}>
                    <div className={classes.cell}>
                        <RandomGifPair namespacePrefix={this.componentManager.fullPath} />
                    </div>
                    <div className={classes.cell}>
                        <ToggleButton namespacePrefix={this.componentManager.fullPath} />
                    </div>
                </div>
                <div>
                    <RandomGifPairPair namespacePrefix={this.componentManager.fullPath} />
                </div>
            </div>

You will also want to dispatch INCREASE_COUNT action as yield effects.put(CounterActions.increaseCount(), "./*"); and add INCREASE_COUNT action to App component's allowedIncomingActionTypes.

  • For Complicated Action Dispatch, you will want to use component Saga. But before that, you will need to adjust namespacePrefix to make sure multicast actions will be received. As namespacePrefix only allows you to receive (listen) actions passing down on one namespace tree branch, you might want to construct a no output component (with saga) with a different namespacePrefix value to capture actions on different namespace tree branch --- in order to avoid this job, the ActionForwarder component was created. It's a very simple component (sourcecode here). You can actually create your own implmentation for your own needs. I think the best example around this usecase is RandomGifPair

2> By looking at your code, the counter component saga looks like doesn't play too much role as main logic has been moved to App. Why not convert it into a pure presentational component?

3> I am very interested in the a chat application use case you've mentioned.
Is it the two-way communication an obstacle to encapsulate the logic?
Will event / action model not suffient for this?
Any chance you can share some examples around any potential issues?

Cheers~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants