-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Extensions: Zoninator - Redirect to zone details instead of zones dashboard after adding a zone #18399
Conversation
|
||
page( `/extensions/zoninator/${ getSiteSlug( getState(), siteId ) }` ); | ||
announceZoneSaved( dispatch, action, fromApi( response.data ) ); | ||
page( `/extensions/zoninator/zone/${ getSiteSlug( getState(), siteId ) }/${ zone.id }` ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm starting to question whether this redirect logic is something that should be hidden inside of the data layer. Is there a compelling reason to not follow the usual process of dispatching an action and redirecting in the component itself?
I feel as though we may have discussed this before? If so, I've forgotten the conclusion. 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we ever settled on anything in this regard.
Personally, I think the data layer gives us the opportunity to treat actions more like events - and I do think the redirection feels more at home in an event handler than a component. But then so would data requests and all the other stuff.
Basically, where I'm coming from - it's the events that power the app, and the state/db is just one of the 'side effects' they produce, as are requests, redirects etc. Obviously, with redux and data-layer it seems to be the other way around - it only handles the state part and everything else is kind of a hack. But that's a topic for a whole another discussion I guess 😄
But I'm still not convinced we should need a render to trigger a response to a successful request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we could do here is to create an "effect handler" that does the redirect indirectly. First, the component would just dispatch an action:
dispatch( pageRedirect( '/extensions/zoninator/zone' );
This doesn't execute anything yet, it just dispatches a PAGE_REDIRECT
action. Tests for the component don't need to mock page
in any way -- they just check that the right action was dispatched.
Then there is an "effect-handler" middleware that intercepts the PAGE_REDIRECT
action and actually executes it.
This is the "effects as data" architecture that fans of languages like Elm enjoy so much.
Another, bigger problem with this data layer code is that it mixes together UI concerns and data concerns. The ZONINATOR_ADD_ZONE
handler doesn't just send the request to the server and update the zones.items
state in Redux. It also displays error notices, redirects to other pages, changes submit state of a form... What does this have to do with data? Nothing. And yet it's hardcoded into the handler. If I want to do the same data operation (add zone) from another UI, I need to create a duplicate handler for that. There, the data layer code is copy&pasted, and only the UI code is different.
My favorite solution would be:
- Make the
ZONINATOR_ADD_ZONE
handler do only the data logic -- the handler knows how to request the data from network, and which part of state to update. Nothing else, especially not anything UI-related. - Make the
dispatch
function call return a promise that can be composed together with the UI logic:
class UIComponent extends Component {
createZone( data ) {
dispatch( startSubmit( this.props.form ) );
dispatch( removeNotice( this.props.saveNotice ) );
dispatch( addZone( data ) )
.then( newZone => {
dispatch( stopSubmit( this.props.form ) );
dispatch( successNotice() );
} )
.catch( error => {
dispatch( stopSubmit( this.props.form ) );
dispatch( errorNotice() );
} );
}
}
Notice how the author of the UI component doesn't have to know any details how addZone
creates the new zone. It might send a HTTP request, it might do something with WebSocket, or it can just create a zone in local storage and sync it to network later.
All the UI does is to tell the data layer: "please create a zone, make sure that the new zone is in state (so that I can retrieve it with a selector) and please notify me (by resolving a promise) when the creation is done, so that I can update the UI".
On the other hand, the UI component knows extremely well how the UI should look like -- and that's why all the UI handling is there, not in the data layer.
Would this API look like an improvement that makes your code better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like an architecturally solid approach -- I'm just wondering if we don't have anything like that already 😄 I could've sworn @dmsnell has advocated for this kind of thing before. Did we ever end up implementing something like this, Dennis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the extended writeup @jsnajdr!
A separate handler for just the redirects seems like the most straight forward approach to me - that's one of the things I was thinking about as well. It will make the handler testable, though it doesn't rally solve the architectural issue you mentioned.
I'd say the approach you outlined above works as long as the notice component is rendered inside the component that triggers the request. Otherwise, we still have to dispatch a redux action and that's a data operation regardless of where it's triggered.
I think one could also make the case for the notifications themselves being data and it being up to the views on how to display them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
been on my TODO list for a long time (and on the lists of several others) but it's a "low priority" because "things are working"
@donpark recently touched on it as well.
I would recommend we draw lots of inspiration from Elm's navigation library as I think it has made the right choices we want in Calypso
but this is a big project too and it's complicated by our use of page.js
middleware. having a basic NAVIGATE
action by itself leaving all other complicated redirects the same is a good starting point I think.
the basic needs are in the new navigation middleware which does two jobs:
- on browser location change events (and load) it dispatches the
NAVIGATION
action which updates app state to reflect it - on
NAVIGATION
actions it updates the browser location
note that our most-prevalent scenario is a bunch of page()
calls in controller.jsx
files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note that our most-prevalent scenario is a bunch of
page()
calls incontroller.jsx
files
Those are route defs (thus touching on routing -- which is yet a more complex terrain) unlike the redirect call we want here. (Let's not try to fix routing by writing our own router -- I'm pretty certain these days that we have stuff like server-side redirects, HTTP status codes, or everything isomorphic that totally warrants researching 3rd party routers.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say the approach you outlined above works as long as the notice component is rendered inside the component that triggers the request. Otherwise, we still have to dispatch a redux action and that's a data operation regardless of where it's triggered.
@ice9js The UI component is completely free to render a global notice by dispatching an errorNotice
action, just like the data layer does. Or it can display it inline in a modal, as we recently did in Simple Payments:
(and I couldn't achieve this using the data layer -- I had to retreat to good old action chunks and direct wpcom
requests)
The point is that it should be the UI that makes the UI decision on how to display the error. It shouldn't be hardcoded in the data layer. Data layer should do only data decisions, that what it's qualified for.
I don't agree that dispatching a Redux action to show a global notice is a data operation. It's an UI operation like any other.
Incidentally, displaying a global notice is the only UI thing that the data layer can actually do. Because it's a global, singleton-style thing and it can be done on any page at any time, regardless of what UI is currently rendered.
b254ec5
to
492fbb6
Compare
dispatch( stopSubmit( form ) ); | ||
dispatch( updateZone( siteId, zone.id, zone ) ); | ||
dispatch( successNotice( translate( 'Zone saved!' ), { id: saveZoneNotice } ) ); | ||
}; | ||
|
||
export const handleZoneCreated = ( { dispatch, getState }, action, response ) => { | ||
const { siteId } = action; | ||
const siteSlug = getSiteSlug( getState(), action.siteId ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we're generally trying to avoid getState()
in actions when easily feasible -- e.g. where the action consumer can pass in the information we'd be otherwise gathering from state. (I've noticed it's used to determine the newly added zone as well.)
Here's the "canonical" source, a SO question with an answer by Redux creator Dan Abramov,
BTW I had to rebase this to be able to |
492fbb6
to
761b3a9
Compare
Hmm, could be my Jetpack site is broken. Hang on... |
Okay, works with the correct Zoninator branch ( |
data: { name: 'Test Zone' }, | ||
}; | ||
|
||
handleZoneCreated( { dispatch, getState }, action, { data: zone } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need to pass getState()
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, action
needs a siteSlug
field now, doesn't it?
}; | ||
|
||
announceZoneSaved( dispatch, action, zone ); | ||
handleZoneCreated( { dispatch, getState }, action, { data: zone } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}; | ||
|
||
announceZoneSaved( dispatch, action, fromApi( zone ) ); | ||
handleZoneCreated( { dispatch, getState }, action, { data: zone } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
}; | ||
|
||
announceZoneSaved( dispatch, action, zone ); | ||
handleZoneCreated( { dispatch, getState }, action, { data: zone } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, looks like tests are broken because of how |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Changes the behaviour after creating a new zone. The user will now be redirected to the details page of the zone he just created rather than zones dashboard as that seems more intuitive.
See p7nzsm-nB-p2.
Testing
/extensions/zoninator
and click Add a zone.