t83714
commented
5 years ago 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
multicastaction (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:
- 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.
- 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-exampleThis way people who're interested in this topic can look at both solutions and probably can benefit more. Cheers~
ghola
Changes to the
fractal-lcomponentimplementation 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:
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 theallowedIncomingMulticastActionTypesarray which would allow you also specify the namespace you want to listen to).randomGifActionTypes.TOGGLED_ONwhich gets dispatched once by theToggleButtoncomponent, and then an additional 2 times by the forwarder (when it should be just once by the forwarder).randomGifActionTypes.NEW_GIFactions withrelativeDispatchPath="../App/*"results in an error: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:
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.