Add pjax:receive event
#384
Conversation
We are using React [1] in combination with pjax. When the content of a pjax container is updated, we need to unmount all React components. Right now, we're doing this in a `pjax:beforeSend` event handler, but that's far from ideal. We need an event to fire right before the content has been updated, while the pjax container still has access to the old content. This commit fixes that by adding a `pjax:receive` event that's fired in the success handler right before we replace the content. The name of the event was inspired by the equivalent event that Turbolinks [2] fires. Closes defunkt#383. [1]: http://facebook.github.io/react/index.html [2]: https://github.com/rails/turbolinks
|
I have no qualms about this. @josh? |
|
Thanks for quick feedback @mislav! We thought about use-cases for intercepting the flow in this event but decided to keep it simple. I think it makes sense though, but it's not something that we have an immediate need for. We're happy to add it if you think it helps others. |
|
Yeah, for now let's not worry about the cancellation behavior. But I'd like you to change something else. Currently, this is rougly what our current xhr success handler does:
It seems that you only need to act before step 10. Could you then move the call to |
|
Given the huge number of steps we have, I'd opt for a For the react case, $(document).on('pjax:replace', function(event, oldContainer) {
React.unmountComponentAtNode(oldContainer)
});
I think thats a little out of this PRs scope. Doing custom insertion is a lot tricker than overriding an event. You also need to implement a way to capture the container for dom caching and how it inserts it from cache on the popstate. We could just mark |
|
O, yeah, this event will also need to fire on |
As mentioned in a comment [1], we want this event to be fired closer to the actual DOM change. A subsequent change might want to make this event cancelable, so that you can provide your own content replacement technique. But for now we're keeping it simple and only allowing listeners to react to the event without interfering. [1]: defunkt#384 (comment)
This pjax lifecycle event is important not only for when an AJAX request changes the container. We also want to allow hooking in to the same place in the lifecycle of popstate events. This commit makes that happen.
As pointed out by @josh in a comment to PR defunkt#384 [1], it would make more sense if this event is called something other than "pjax:receive". We initially picked that name to stay consistent with Turbolinks event naming, but now that we've moved the event to a slightly different place in the pjax lifecycle, the name could be more descriptive. Looking at the names of the events in the readme, we think that "pjax:beforeReplace" is a good descriptive name. [1]: defunkt#384 (comment)
|
We've pushed some changes in a couple of follow-up commits. We added the event to the popstate handler and renamed it to "pjax:beforeSend". Let us know what you think. A natural follow-up would probably be to allow this event to cancel execution, thus allowing you do to the replacing yourself. I know that @lencioni was interested in using the DOM-updating capabilities of React to replace content, similar to what react-magic (https://github.com/reactjs/react-magic) does. |
jquery.pjax.js
Outdated
There was a problem hiding this comment.
null data seems like an api smell.
There was a problem hiding this comment.
Yeah, I guess I was just following the local pattern (see line 444 and 451).
There was a problem hiding this comment.
We pass null to pjax:start/end because they might fire in the context of an xhr request, but they can also fire on popstate where there is no xhr object. The handler method should check if xhr truthy or not before consuming the xhr object.
Here the scenario is slighly different. In the first case, I would pass the container object to pjax:beforeReplace so that the event handler could potentially manipulate the container object.
But, if we do that, then we would have to pass a similar object to pjax:beforeReplace on popstate, but we don't have such an object. So in both cases, we could pass the raw content as the 1st argument (container.contents in the first case, contents in the second)
There was a problem hiding this comment.
The start/end case feels like a bad legacy decision we had to deal with. Like you said, here its not a xhr, its a different pattern.
For this "unmount" use case you still need to run your unmount function on popstate as well. I feel like we should just pass in the contents in both places so the arguments are consistent. Skip raw access to data.
There was a problem hiding this comment.
Alright, I'll go ahead and pass through contents. Thanks for making sure the quality is kept high.
Before, we would send different data along with the pjax:beforeReplace depending on when the event was fired. When fired in the context of an AJAX request, the response `data` from the server would be passed along. In the context of a popstate event, we didn't have access to the response from the server, so we just passed along `null` in place of the server response data. @josh and @mislav suggested that we pass along the contents that we are about to inject into the pjax container. We think that's a good idea, it will allow people to inspect the new content before it's replaced. And when we make this event cancelable in the future, we could allow people to implement their own content-replacement strategy.
4 participants
We are using React 1 in combination with pjax. When the content of a
pjax container is updated, we need to unmount all React components.
Right now, we're doing this in a
pjax:beforeSendevent handler, butthat's far from ideal. We need an event to fire right before the content
has been updated, while the pjax container still has access to the old
content. This commit fixes that by adding a
pjax:receiveevent that'sfired in the success handler right before we replace the content.
The name of the event was inspired by the equivalent event that
Turbolinks 2 fires.
Closes #383.