Hooks: Dispatch all heartbeat events as hooks actions #11781
Conversation
aduth
added
[Type] Code Quality
|
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:
|
gziolo
added
the
[Priority] Low
aduth
force-pushed
the
remove/editor-jquery
branch
from
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.
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.
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.
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.
Should we consider the
heartbeat.errorevent/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.tickdoesn'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.
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.
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.
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.
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.
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.
| 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.
| 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.
| addAction( 'heartbeat.tick', hookNamePrefix + '-tick', this.receivePostLock ); | |
| addAction( 'heartbeat.tick', hookNamePrefix, this.receivePostLock ); |
There was a problem hiding this comment.
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 🤷♂️ |
aduth
force-pushed
the
remove/editor-jquery
branch
from
eed0bd8
to
75793f2
Compare
aduth
force-pushed
the
remove/editor-jquery
branch
from
75793f2
to
454c93f
Compare
Since it tests for static method on which to call
aduth
force-pushed
the
remove/editor-jquery
branch
from
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
editormodule 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-fetchnonce middleware.