Support page messaging #49
Conversation
There was a problem hiding this comment.
This is awesome! Thanks for doing this. I'd love to verify that this is working on the examples without an extension id passed in.
Also, we should make sure to update the unit tests:
https://github.com/tshaddix/react-chrome-redux/blob/master/test/Store.test.js
|
|
||
| this.port = chrome.runtime.connect({name: portName}); | ||
| this.extensionId = extensionId; //keep the extensionId as an instance variable | ||
| this.port = chrome.runtime.connect(this.extensionId, {name: portName}); |
There was a problem hiding this comment.
@godd9170 Have you verified this works when it is passed in as an empty string? We should test on the examples repo :)
There was a problem hiding this comment.
I have yes, I tried it with your original clicker-key example happening in the content.js.
There was a problem hiding this comment.
Okay, great. I will do it on my end as well so we can get two confirmations involved 👍
| * Setup for state updates | ||
| */ | ||
| const connectState = (port) => { | ||
| console.log('connected on port: ', port); |
There was a problem hiding this comment.
Can we remove this console log?
There was a problem hiding this comment.
Did I put that one in? Sorry I thought that was from the original. Sure can.
| @@ -64,7 +66,9 @@ class Store { | |||
| */ | |||
| dispatch(data) { | |||
| return new Promise((resolve, reject) => { | |||
| chrome.runtime.sendMessage({ | |||
| chrome.runtime.sendMessage( | |||
| this.extensionId, | |||
There was a problem hiding this comment.
Have we verified this works with an empty extension id?
| /** | ||
| * Setup action handler | ||
| */ | ||
| chrome.runtime.onMessage.addListener((request, sender, sendResponse) => { |
There was a problem hiding this comment.
Can we simplify this as chrome.runtime.onMessage.addListener(dispatchResponse)
There was a problem hiding this comment.
Seems pretty reasonable!
| * Setup external action handler | ||
| */ | ||
| chrome.runtime.onMessageExternal.addListener((request, sender, sendResponse) => { | ||
| return dispatchResponse(request, sender, sendResponse); |
| * Setup extended connection | ||
| */ | ||
| chrome.runtime.onConnect.addListener((port) => { | ||
| return connectState(port); |
There was a problem hiding this comment.
Can we simplify this to chrome.runtime.onConnect.addListener(connectState)
| * Setup extended external connection | ||
| */ | ||
| chrome.runtime.onConnectExternal.addListener((port) => { | ||
| return connectState(port); |
|
@tshaddix you mean to link to a PR? I think you linked to a file :-P |
|
@vhmth Oh sorry yes I meant to link to a file. I was showing where the unit test needed to be changed. I understand the confusion by saying "this PR" :P Fixed |
|
@tshaddix glad you like it, apologies about breaking all your tests, I was too tired to do them last night! I'll make those changes later tonight, do you also want me to take a stab at re-writing the unit tests? I'm afraid to admit I've never done any javascript unit testing, but keen to learn. |
|
@godd9170 Absolutely no worries! A lot of good additions in here :) It's up to you! I think it's a good way to round off this PR and the problem space if pretty small, so it might be good place for you to try it out! If not, I'll be happy to fix them up. Just let me know and I'll help where I can! |
|
Ok, changes applied and tests fixed. I think we should probably mock up the |
|
Awesome work fixing up the tests! Going to run this one more time on my end against the examples and then I think we can merge. |
|
@godd9170 Your changes have been released in |
I've added an optional
extensionIdto theStoreconstructor. I use the extensionId in theconnectandsendMessagefunctions, and listen foronMessageExternalandonConnectExternalin thebackground.js. fixes #47