Skip to content
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

Rename yieldToMain to splitTask and export from @wordpress/interactivity #62665

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

westonruter
Copy link
Member

@westonruter westonruter commented Jun 18, 2024

Follow up to #61885

I just realized that the new yieldToMain function isn't currently exported by @wordpress/interactivity. This should be exported because it allows for directives to reuse this logic to break up their own long tasks when they can't make use of the wp-async directive (or even if they do and still are doing a lot of work).

For example, consider this synchronous wp-on directive action which calls event.preventDefault() but then does work.

import { store, splitTask } from '@wordpress/interactivity';

store( 'foo', {
	actions: {
		showSomething: function* ( event ) => {
			event.preventDefault();
			showLoadingSpinner();
			yield splitTask();
			const data = fetchData();
			processData( data );
			yield splitTask();
			renderData( data );
		}
} );

(Granted: A developer could just re-implement yieldToMain on their own.)

If approved and backported, this will need to be mentioned in the doc updates in #62663, in particular under Async Actions.

Copy link

github-actions bot commented Jun 18, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: luisherranz <luisherranz@git.wordpress.org>
Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@westonruter westonruter requested a review from cbravobernal June 18, 2024 18:25
@westonruter westonruter added [Type] Enhancement A suggestion for improvement. [Packages] Interactivity /packages/interactivity labels Jun 18, 2024
@westonruter westonruter force-pushed the export/yield-to-main branch from 6158bd4 to e6cc8b2 Compare June 18, 2024 19:05
@westonruter westonruter added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jun 18, 2024
@westonruter westonruter force-pushed the export/yield-to-main branch from e6cc8b2 to 7c17304 Compare June 18, 2024 19:11
@westonruter
Copy link
Member Author

Humm. In a generator context, the yieldToMain() name doesn't look so good:

yield yieldToMain();

@westonruter westonruter removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jun 18, 2024
@westonruter
Copy link
Member Author

For now I've manually copied the yieldToMain() function into the Async Actions docs section in #62663.

Copy link
Member

@luisherranz luisherranz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Humm. In a generator context, the yieldToMain() name doesn't look so good:

yield yieldToMain();

I don't think it's that bad, but feel free to rename 🙂


By the way, shouldn't we also update this section of the documentation? Or do you want to make those changes in another pull request?

As mentioned above with [`wp-on`](#wp-on), [`wp-on-window`](#wp-on-window), and [`wp-on-document`](#wp-on-document), an async action should be used whenever the `async` versions of the aforementioned directives cannot be used due to the action requiring synchronous access to the `event` object. Synchronous access is reqired whenever the action needs to call `event.preventDefault()`, `event.stopPropagation()`, or `event.stopImmediatePropagation()`. To ensure that the action code does not contribute to a long task, you may manually yield to the main thread after calling the synchronous event API. For example:
```js
function toMainThread() {
return new Promise(resolve => {
setTimeout(resolve, 0);
});
}
store("myPlugin", {
actions: {
handleClick: function* (event) {
event.preventDefault();
yield toMainThread();
doTheWork();
},
},
});
```

@cbravobernal cbravobernal added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Jun 19, 2024
@cbravobernal
Copy link
Contributor

Docs have been merged with toMainThread, maybe we can rename it to that.

@luisherranz
Copy link
Member

Docs have been merged with toMainThread, maybe we can rename it to that.

Up to you, folks 🙂

@westonruter
Copy link
Member Author

westonruter commented Jun 20, 2024

Or simpler, what about just mainThread? Usage:

await mainThread();
yield mainThread();

I like this because the preposition "to" doesn't work with "await" in English (one does not just "await to something") although "to" is used with "yield" here. By removing the preposition entirely, then this awkwardness is removed. Also, by omitting a verb from the function name itself it implies that the function doesn't actually do anything on its own. Indeed, it must be used with await or yield to have any purpose.

@westonruter
Copy link
Member Author

I checked with @tunetheweb and he suggested splitTask() instead.

@westonruter westonruter changed the title Export yieldToMain from @wordpress/interactivity Rename yieldToMain to splitTask and export from @wordpress/interactivity Jun 20, 2024
@westonruter westonruter requested a review from ndiego as a code owner June 20, 2024 20:47
@@ -813,7 +813,8 @@ const { state } = store("myPlugin", {
As mentioned above with [`wp-on`](#wp-on), [`wp-on-window`](#wp-on-window), and [`wp-on-document`](#wp-on-document), an async action should be used whenever the `async` versions of the aforementioned directives cannot be used due to the action requiring synchronous access to the `event` object. Synchronous access is reqired whenever the action needs to call `event.preventDefault()`, `event.stopPropagation()`, or `event.stopImmediatePropagation()`. To ensure that the action code does not contribute to a long task, you may manually yield to the main thread after calling the synchronous event API. For example:

```js
function toMainThread() {
// Note: In WordPress 6.6 this splitTask function is exported by @wordpress/interactivity.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This note assumes this change will be backported.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, shouldn't we also update this section of the documentation? Or do you want to make those changes in another pull request?

@luisherranz This comment in the docs here I believe updates the section accordingly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fantastic, thank you.

@westonruter
Copy link
Member Author

@cbravobernal I'll let you merge if you are satisfied.

@cbravobernal cbravobernal merged commit 96a2ebd into trunk Jun 21, 2024
69 checks passed
@cbravobernal cbravobernal deleted the export/yield-to-main branch June 21, 2024 10:56
@github-actions github-actions bot added this to the Gutenberg 18.7 milestone Jun 21, 2024
@DAreRodz
Copy link
Contributor

Nice addition. 😄 Not sure, but maybe we need to add an entry log for the splitTask() function, right?

@luisherranz
Copy link
Member

luisherranz commented Jun 24, 2024

Oh, yes please!

@westonruter
Copy link
Member Author

@DAreRodz @luisherranz Like #62805?

luisherranz pushed a commit that referenced this pull request Jun 25, 2024
#62805)

* Add changelog entry for splitTask

Add changelog entry for #62665

* Move changelog entry to Unreleased section

Co-authored-by: Carlos Bravo <37012961+cbravobernal@users.noreply.github.com>

---------

Co-authored-by: Carlos Bravo <37012961+cbravobernal@users.noreply.github.com>
@luisherranz
Copy link
Member

Merged. Thanks Weston!

ellatrix pushed a commit that referenced this pull request Jun 25, 2024
…ity (#62665)

* Export yieldToMain from @wordpress/interactivity

* Rename yieldToMain to splitTask

Co-authored-by: tunetheweb <tunetheweb@git.wordpress.org>

* Update docs to note splitTask being exported

---------

Co-authored-by: tunetheweb <tunetheweb@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: luisherranz <luisherranz@git.wordpress.org>
Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org>
@ellatrix
Copy link
Member

I just cherry-picked this PR to the wp/6.6-rc-1 branch to get it included in the next release: f37faf7

@ellatrix ellatrix added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Jun 25, 2024
ellatrix pushed a commit that referenced this pull request Jun 25, 2024
…ity (#62665)

* Export yieldToMain from @wordpress/interactivity

* Rename yieldToMain to splitTask

Co-authored-by: tunetheweb <tunetheweb@git.wordpress.org>

* Update docs to note splitTask being exported

---------

Co-authored-by: tunetheweb <tunetheweb@git.wordpress.org>
Co-authored-by: westonruter <westonruter@git.wordpress.org>
Co-authored-by: luisherranz <luisherranz@git.wordpress.org>
Co-authored-by: cbravobernal <cbravobernal@git.wordpress.org>
peterwilsoncc pushed a commit to peterwilsoncc/gutenberg-build that referenced this pull request Sep 11, 2024
…y (#62805)

* Add changelog entry for splitTask

Add changelog entry for WordPress/gutenberg#62665

* Move changelog entry to Unreleased section

Co-authored-by: Carlos Bravo <37012961+cbravobernal@users.noreply.github.com>

---------

Co-authored-by: Carlos Bravo <37012961+cbravobernal@users.noreply.github.com>

Source: WordPress/gutenberg@1ba8018
peterwilsoncc pushed a commit to peterwilsoncc/gutenberg-build that referenced this pull request Sep 12, 2024
…y (#62805)

* Add changelog entry for splitTask

Add changelog entry for WordPress/gutenberg#62665

* Move changelog entry to Unreleased section

Co-authored-by: Carlos Bravo <37012961+cbravobernal@users.noreply.github.com>

---------

Co-authored-by: Carlos Bravo <37012961+cbravobernal@users.noreply.github.com>

Source: WordPress/gutenberg@1ba8018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Backported to WP Core Pull request that has been successfully merged into WP Core [Packages] Interactivity /packages/interactivity [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants