[Resource Picker] Fix reference to old callback function

[katzenbar]

WHY are these changes introduced?

When developing an app that adds line items to a draft order, I noticed that my ResourcePicker kept resetting my line items to the initial state. Since this component requires being inside of an embedded app and I am working on a private project, I put together an example app that demonstrates the issue: https://github.com/katzenbar/shopify-resource-picker-bug

This example app takes the products selected in the ResourcePicker, and adds them to the end of the list in the card. https://github.com/katzenbar/shopify-resource-picker-bug/blob/master/app/javascript/components/ProductPage.tsx#L24-L28

But when handleSelection is run, the products array is empty. This results in the old selection being replaced.

The expected behavior should be that the newly selected products are added to the end of the list.

I believe part of reproducing this issue is using React Hooks instead of setState, but I have not tested to verify that is the case.

WHAT is this pull request doing?

When pulling out onSelection and onCancel in componentDidMount through destructuring, you will keep a reference to the version of that function when the component was mounted. If data these functions reference is updated, these function references become stale and are not updated. In theory, if you were to initially have these callbacks be null and then change them to a real function they would never get called.

To fix this, I changed the code to always register a subscriber for onSelection and onCancel and get the current reference of these from this.props when the callback fires. This seems more efficient that trying to track subscribe/unsubscribe inside of componentDidUpdate in the general case. I've tested this on my sample application in Chrome, and it works as I expect now.

If anyone else is experiencing this issue, a workaround is to treat the ResourcePicker like the Toast example in the documentation and only render it when you are about to open the modal.

{showResourcePicker && (
        <ResourcePicker
          resourceType="Product"
          initialQuery={productSearchQuery}
          open={showResourcePicker}
          onSelection={handleResourcePickerSelection}
          onCancel={closeResourcePicker}
        />
      )}

[ghost]

👋 Thanks for opening your first pull request. A contributor should give feedback soon. If you haven’t already, please check out the contributing guidelines. You can also join #polaris on the Shopify Partners Slack.

[tmlayton]

Thanks for working on this fix! Instead of keeping the reference to this.props in the subscribed callback, let’s unsubscribe and resubscribe the updated callbacks in componentDidUpdate.

  componentDidUpdate() {
    const {onSelection, onCancel} = this.props;

    this. appBridgeResourcePicker.unsubscribe();
    if (onSelection != null) {
      this. appBridgeResourcePicker.subscribe(AppBridgeResourcePicker.Action.SELECT, onSelection);
    }

    if (onCancel != null) {
      this. appBridgeResourcePicker.subscribe(AppBridgeResourcePicker.Action.CANCEL, onCancel);
    }
  }

Also, add a test or two to get the coverage > 90%. Happy to help with this if needed!

[katzenbar]

@tmlayton - Updated PR to address feedback. Sorry about not getting back to this quickly, I caught a nasty cold last week!

[tmlayton]

Just a couple minor changes to the tests to expect that we call unsubscribe, otherwise this looks great, nice work and thanks for the contribution!

[katzenbar]

@tmlayton Updated

[tmlayton]

Looks great, thanks for the contribution!

[ghost]

🎉 Thanks for your contribution to Polaris React!