Skip to content
This repository has been archived by the owner on Dec 3, 2020. It is now read-only.

Centralize message handling to allow for multiple async listeners. #170

Merged
merged 2 commits into from
Oct 18, 2018

Conversation

Osmose
Copy link
Contributor

@Osmose Osmose commented Oct 16, 2018

This is the messaging refactor extracted from #168.

@Osmose Osmose requested a review from biancadanforth October 16, 2018 20:39
Copy link
Collaborator

@biancadanforth biancadanforth left a comment

Choose a reason for hiding this comment

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

R+WC. Very clean! Extraction doesn't work right now due to a super minor change needed. Be sure to make that change before merging.

*/

import {handleConfigMessage} from 'commerce/config/background';
import {handleExtractedProductData} from 'commerce/background/extraction';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Extraction doesn't work right now for a very minor reason:

  • The handleExtractedProductData method needs to be updated, since you are now sending message instead of message.extractedProduct as the first argument.

Currently:

export async function handleExtractedProductData(contentExtractedProduct, sender) {
  // Fetch the favicon, which the content script cannot do itself.
  const extractedProduct = {
    ...contentExtractedProduct,
    vendorFaviconUrl: sender.tab.favIconUrl,
  };
// ...

Change that makes it work:

export async function handleExtractedProductData(message, sender) {
  // Fetch the favicon, which the content script cannot do itself.
  const extractedProduct = {
    ...message.extractedProduct,
    vendorFaviconUrl: sender.tab.favIconUrl,
  };
// ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, my bad.

return handlers.get(message.type)(message, sender);
}

return undefined;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain a little more why we want to return undefined here? I'm not sure I'm totally following.

Would we not want to console.warn or something that the message.type is not recognized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ESLint doesn't like if we have a return statement in some branches but not others. Since the if has a return, we need one outside too in case we don't return there. undefined is the default return value for functions that don't return, so I'm just using that here as well.

@Osmose Osmose merged commit 30b168a into master Oct 18, 2018
@Osmose Osmose deleted the message-refactor branch October 18, 2018 21:01
@Osmose
Copy link
Contributor Author

Osmose commented Oct 18, 2018

Thank you!

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.

2 participants