-
Notifications
You must be signed in to change notification settings - Fork 4.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
Hooks: Dispatch all heartbeat events as hooks actions #11781
Conversation
I noticed two regressions. This error message was triggered on the original editor when I "took over editing". I also noticed that releasing a post is not working properly (I've used Firefox to release the post, I did'tn try Chrome). So even if the original author is not editing the post anymore, the second author see the lock modal. |
I don't suppose we have automated testing for this feature then? 😅 |
I actually wrote the e2e tests and committed them in the original PR but had to revert them because I was not able to push them to a stable state. |
I pushed two commits which should resolve your two issues:
|
9d2c3ef
to
eed0bd8
Compare
Moving this to 4.6, no rush to get it in today. |
lib/client-assets.php
Outdated
'heartbeat-nonces-expired', | ||
].join( ' ' ), function( event ) { | ||
var actionName = event.type.replace( /-/g, '.' ); | ||
wp.hooks.doAction.apply( null, [ actionName ].concat( [].slice.call( arguments, 1 ) ) ); |
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 add a comment here along the lines of
wp.hooks.doAction.apply( null, [ actionName ].concat( [].slice.call( arguments, 1 ) ) ); | |
// Heartbeat API events have different number of arguments passed to them, | |
// so we need to destructure them to pass them to the proper action hook. | |
wp.hooks.doAction.apply( null, [ actionName ].concat( [].slice.call( arguments, 1 ) ) ); |
I wish I could have found a link explaining the different Heartbeat action hooks and their payload to add to the comment, but I couldn't.
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 wish I could have found a link explaining the different Heartbeat action hooks and their payload to add to the comment, but I couldn't.
There's no written documentation, but there's a code file from which the original list was obtained. We could potentially include this (will have to find it once more).
@@ -30,17 +30,30 @@ class PostLockedModal extends Component { | |||
} | |||
|
|||
componentDidMount() { |
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.
Should we consider the heartbeat.error
event/hook as well? I'm not that familiar with the heartbeat API so I don't know in what specific situation that would be fired that heartbeat.tick
doesn't cover. At first sight, it seems sensible. If that makes sense, I'm happy if we do that in a follow-up PR and I can prepare one.
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.
Should we consider the
heartbeat.error
event/hook as well? I'm not that familiar with the heartbeat API so I don't know in what specific situation that would be fired thatheartbeat.tick
doesn't cover. At first sight, it seems sensible. If that makes sense, I'm happy if we do that in a follow-up PR and I can prepare one.
Maybe, but as you alluded, I'm more concerned on an in-place-compatible refactoring here; no additions of behavior.
*/ | ||
getHookNamePrefix() { | ||
const { instanceId } = this.props; | ||
return 'core/editor/post-locked-modal-' + instanceId + '-heartbeat'; |
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 like this namespace better. Yet I find the heartbeat-{send/tick}
repetition not necessary as it is already in the action name. How about:
return 'core/editor/post-locked-modal-' + instanceId + '-heartbeat'; | |
return 'core/editor/post-locked-modal-' + instanceId; |
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.
FWIW, that would be more in line with the other existing heartbeat use case (nonce middleware) where we use core/api-fetch/create-nonce-middleware
.
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.
Sure 👍 Since the function name needs to be provided when calling removeAction
anyways (the theoretical need for this in the first place), it's arguably redundant.
jQuery( document ) | ||
.on( 'heartbeat-send.refresh-lock', this.sendPostLock ) | ||
.on( 'heartbeat-tick.refresh-lock', this.receivePostLock ); | ||
addAction( 'heartbeat.send', hookNamePrefix + '-send', this.sendPostLock ); |
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.
As I mentioned here, how about:
addAction( 'heartbeat.send', hookNamePrefix + '-send', this.sendPostLock ); | |
addAction( 'heartbeat.send', hookNamePrefix, this.sendPostLock ); |
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 guess, the Suggest change
beta feature doesn't like multicomment lines :/ Edited.
.off( 'heartbeat-tick.refresh-lock', this.receivePostLock ); | ||
const hookNamePrefix = this.getHookNamePrefix(); | ||
|
||
removeAction( 'heartbeat.send', hookNamePrefix + '-send' ); |
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.
removeAction( 'heartbeat.send', hookNamePrefix + '-send' ); | |
removeAction( 'heartbeat.send', hookNamePrefix ); |
const hookNamePrefix = this.getHookNamePrefix(); | ||
|
||
removeAction( 'heartbeat.send', hookNamePrefix + '-send' ); | ||
removeAction( 'heartbeat.tick', hookNamePrefix + '-tick' ); |
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.
removeAction( 'heartbeat.tick', hookNamePrefix + '-tick' ); | |
removeAction( 'heartbeat.tick', hookNamePrefix ); |
.on( 'heartbeat-send.refresh-lock', this.sendPostLock ) | ||
.on( 'heartbeat-tick.refresh-lock', this.receivePostLock ); | ||
addAction( 'heartbeat.send', hookNamePrefix + '-send', this.sendPostLock ); | ||
addAction( 'heartbeat.tick', hookNamePrefix + '-tick', this.receivePostLock ); |
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.
addAction( 'heartbeat.tick', hookNamePrefix + '-tick', this.receivePostLock ); | |
addAction( 'heartbeat.tick', hookNamePrefix, this.receivePostLock ); |
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've left some minor comments that I'd be happy if were addressed. Not a blocker.
This is working as expected. Worth noting that in my testing, the autosave network request on receivePostLock
has failed both in this PR and in master
. I will follow up with an issue for that if there isn't one already.
No longer can repro the issue 🤷♂️ |
eed0bd8
to
75793f2
Compare
75793f2
to
454c93f
Compare
Since it tests for static method on which to call
454c93f
to
e1c3c0e
Compare
This needs a Trac ticket, since it must be changed in core in time for 5.2 (5.1 aligns to Gutenberg 4.8, so the version prior to what included this change). (Will set a reminder for myself to create one) |
It appears this should already be accounted for, as part of the following changeset: https://core.trac.wordpress.org/changeset/43939 But then it also occurs to me that Gutenberg's proxying may now actually cause the action to be handled twice, once for the native If the above changeset was included as part of WordPress 5.0, it should stand to reason that we might not need the jQuery event proxying at all? |
* Framework: Dispatch all heartbeat events as actions * Editor: Send PostLockModal lock request via XHR * Editor: Use hooks for PostLockedModal heartbeat handling * Framework: Update package-lock.json per jQuery dependency drop * Editor: Remove unsupported event object from action callback * Editor: Assure withGlobalEvents is last to wrap component Since it tests for static method on which to call * Editor: Reuse hook name across disparate hooks
* Framework: Dispatch all heartbeat events as actions * Editor: Send PostLockModal lock request via XHR * Editor: Use hooks for PostLockedModal heartbeat handling * Framework: Update package-lock.json per jQuery dependency drop * Editor: Remove unsupported event object from action callback * Editor: Assure withGlobalEvents is last to wrap component Since it tests for static method on which to call * Editor: Reuse hook name across disparate hooks
Related: #4217
This pull request seeks to enhance our handling of heartbeat-to-hooks upgrade to consistently fire all heartbeat events as actions. In addition to promoting consistency, it enables us to refactor the PostLockedModal to avoid having the
editor
module depend on jQuery.Testing instructions:
Verify there are no regressions in the behavior of post locking by repeating testing instructions from #4217.
Verify there are no regressions in other existing usage of upgrade heartbeat events, i.e. the
api-fetch
nonce middleware.