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

Fix explanation in runtime.onMessage doc #3903

Merged
merged 2 commits into from
May 21, 2021

Conversation

sideshowbarker
Copy link
Member

Fixes #1452

@sideshowbarker sideshowbarker requested a review from a team as a code owner April 7, 2021 03:48
@sideshowbarker sideshowbarker requested review from muffinresearch and removed request for a team April 7, 2021 03:48
@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2021

Preview URLs

Flaws

URL: /en-US/docs/Mozilla/Add-ons/WebExtensions/API/runtime/onMessage
Title: runtime.onMessage
on GitHub
Flaw count: 3

  • broken_links:
    • No need for the pathname in anchor links if it's the same page
    • No need for the pathname in anchor links if it's the same page
    • No need for the pathname in anchor links if it's the same page

External URLs

URL: /en-US/docs/Mozilla/Add-ons/WebExtensions/API/runtime/onMessage
Title: runtime.onMessage
on GitHub

No new external URLs

(this comment was updated 2021-05-19 07:08:35.068293)

@sideshowbarker sideshowbarker changed the title Fix explanation of in runtime.onMessage doc Fix explanation in runtime.onMessage doc Apr 8, 2021
@sideshowbarker sideshowbarker force-pushed the sideshowbarker/runtime.onMessage-fix branch from 511975c to 1ce9000 Compare April 8, 2021 06:11
@sideshowbarker sideshowbarker requested a review from a team April 19, 2021 03:26
@chrisdavidmills chrisdavidmills removed their request for review May 7, 2021 07:11
@chrisdavidmills
Copy link
Contributor

I think I'll leave this one to @muffinresearch , as it's more his area of specialty. You OK to review this one Stuart?

@muffinresearch muffinresearch requested review from rpl and removed request for muffinresearch May 10, 2021 10:37
@muffinresearch
Copy link

These changes look ok to me but I'll defer this review to @rpl

Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

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

@sideshowbarker my apologies for not reaching this sooner.

The new wording seems to be at least slightly better, but I have the feeling that it is still a bit open to be misread.

Let's see if we can improve it a bit more.
@sideshowbarker wdyt about the tweaks proposed below?

(by knowing in detail how that works on the implementation side does likely make me unintentionally biased on evaluating how clear it really is from an external reader perspective, and so your perspective on the tweaks I'm proposing is important as much as mine, if not even more).

@@ -111,7 +111,7 @@ <h3 id="Parameters">Parameters</h3>
<p>The <code><var>listener</var></code> function can return either a Boolean or a {{jsxref("Promise")}}.</p>

<div class="notecard note">
<p><strong>Important:</strong> Do not call <code>addListener()</code> using an <code>async</code> function:</p>
<p><strong>Important:</strong> If you call <code>addListener()</code> using an <code>async</code> function, the listener will return a Promise for every message it receives, preventing other listeners from responding:</p>
Copy link
Member

Choose a reason for hiding this comment

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

Nit, I'm wondering if saying here If you pass to addListener an async function instead of If you call addListener using an async function would make it more clear that async function is referring to the parameter we pass to addListener not the function inside which we may be calling addListener.

@muffinresearch what do you think? I would like to hear your pov from an English form perspective (because I may be biased by how my brain automatically translate that phrase in my own language).

Choose a reason for hiding this comment

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

I would suggest changing that slightly from If you pass to addListener an async function to If you pass an async function to addListener.

Copy link
Member

Choose a reason for hiding this comment

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

yep, that does sound better 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

@rpl, @muffinresearch, thanks — change that line so that it now reads:

If you pass an async function to addListener(), the listener will return a Promise for every message it receives, preventing other listeners from responding

<p>This will cause the listener to consume every message it receives, effectively blocking all other listeners from receiving and processing messages.</p>

<p>If you want to take an asynchronous approach, use a Promise instead, like this:</p>
<p>If you only want the listener to respond to messages of a certain type, you must check the message type synchronously before returning a Promise:</p>
Copy link
Member

Choose a reason for hiding this comment

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

Nit, you must define the listener as a non-<code>async</code> function, return a Promise only for the messages the listener is meant to respond to, and undefined or false otherwise?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks — changed that line so that it now reads:

If you only want the listener to respond to messages of a certain type, you must define the listener as a non-async function, and return a Promise only for the messages the listener is meant to respond to — and otherwise return false or undefined

@sideshowbarker sideshowbarker force-pushed the sideshowbarker/runtime.onMessage-fix branch from 1ce9000 to 074b574 Compare May 19, 2021 07:06
Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

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

@sideshowbarker Thanks, this version seems much more clear to me. r+ from my side.

@caitmuenster do you want to give this a quick look from an editorial perspective?

@caitmuenster
Copy link
Contributor

This looks good to me! Thank you so much, @sideshowbarker!

@sideshowbarker
Copy link
Member Author

Thanks all — gonna go ahead and merge now per #3903 (comment)

@sideshowbarker sideshowbarker merged commit cbf690c into main May 21, 2021
@sideshowbarker sideshowbarker deleted the sideshowbarker/runtime.onMessage-fix branch May 21, 2021 00:54
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing warning about async runtime.onMessage listeners
5 participants