Conditional check external message support #60
Conversation
Web Extension support is broken because Firefox does not support `external` namespace messaging. Added checks in `wrapStore` to make sure the external listeners are actually defined before adding. Fixes #54
This is a bug fix.
| @@ -99,7 +99,9 @@ export default (store, { | |||
| /** | |||
| * Setup external action handler | |||
| */ | |||
| chrome.runtime.onMessageExternal.addListener(dispatchResponse); | |||
| if (chrome.runtime.onMessageExternal) { | |||
There was a problem hiding this comment.
Does chrome.runtime exist in FF? I'm assuming it does, but maybe we should create a utility file called features that has functions like:
export const hasMessageExternal = () => {
return chrome && chrome.runtime && chrome.runtime.onMessageExternal;
};
That way, when we want to support other browsers (like IE Edge), we can simply change the logic in the functions once and it should largely apply across the libray.
There was a problem hiding this comment.
@vhmth it looks like in FF it's browser.runtime as seen here: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/runtime/getBackgroundPage
Love the idea of a utility file for features.
There was a problem hiding this comment.
@vhmth @paulius005 Great idea for a utility file. The chrome.* namespace is provided in Firefox currently in order to help developers port extensions over from Chrome painlessly.
From
https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities :
"As a porting aid, the Firefox implementation of WebExtensions supports chrome and callbacks as well as browser and promises."
On the other hand, Microsoft as usual is going for the lovely land of polyfills, with the need to add additional scripts from them in your extension folder in order to make existing chrome extensions work: https://docs.microsoft.com/en-us/microsoft-edge/extensions/guides/porting-chrome-extensions
There was a problem hiding this comment.
Great info here @AgrimPrasad. I think if polyfills are required in the IE case, we focus on FF first and then tackle IE separately. We could do something in the future like dynamically running polyfills for IE or create a separate compatibility repo that will detect the browser and bridge any API differences. Do you happen to know off the top of your head if there are efforts going into a lib like that?
There was a problem hiding this comment.
@vhmth Off the top of my head, no, I don't know of any existing library which does or aims to do that. I like the idea of a separate library which would be general enough to fit all extensions, rather than be specific to react-chrome-redux... Fits the unix philosophy of a utility doing one thing well.
There was a problem hiding this comment.
Onion-layering. I agree that's the best. We can leave it here as a utility so we can play with it and test with react-chrome-redux and then create a separate lib as it gets some field-testing :-)
There was a problem hiding this comment.
Actually, on thinking more about react-chrome-redux specifically, I feel that it would be quicker (and simpler) at this point for us to use browser.* api with the Mozilla supplied polyfill for Chrome. I've been using this polyfill in my extension project, and so far it has worked flawlessly in Chrome for me.
This would mean:
react-chrome-reduxworks out of the box with Firefox and Edge.react-chrome-reduxcontinues to work without issues in Chrome.
A separate library which assists in the porting of Chrome extensions to WebExtensions would still be useful for other projects which are more invested in the chrome.* namespace, but it seems like an unnecessary overkill for react-chrome-redux
What do you guys think?
There was a problem hiding this comment.
Hmm seems like the first action item for that is to document all the cases where we use chrome.* APIs and see if those have a polyfill. With sweeping cross-platform plays like that, we lock ourselves in to only use the APIs which exist on all platforms, which I'm cool with if @tshaddix is, but I would be more comfortable with that being handled in a larger PR that gets tested with our own Chrome extension and others just to make sure everything is peachy.
There was a problem hiding this comment.
Well... this took off. Amazing what a couple if statements can do :P
I'm a little torn in this discussion. In some ways, I'd love for react-chrome-redux to mold and fit whatever shape it needs to be in for the developer. In another way, I feel like polyfilling should be left up to the implementor.
I feel like we should move this to an issue to discuss. It's a great discussion and we should continue it.
This PR was meant to unblock @AgrimPrasad :)
There was a problem hiding this comment.
Agreed I'm of the belief that polyfills should be included by the implementor too actually. I'm cool with just going ahead with this PR closing to help out @AgrimPrasad, but maybe leave a comment as to why we check the existence of that function?
| @@ -109,6 +111,7 @@ export default (store, { | |||
| /** | |||
| * Setup extended external connection | |||
| */ | |||
| chrome.runtime.onConnectExternal.addListener(connectState); | |||
|
|
|||
| if (chrome.runtime.onConnectExternal) { | |||
There was a problem hiding this comment.
It would be user friendly if an else clause could be added here with a console.log to notify Firefox users that a feature (which one, I'm not sure... @tshaddix, you should know) from react-chrome-redux is unavailable.
There was a problem hiding this comment.
I agree. The feature was the ability to have scripts running on the website itself communicate with the background store.
There was a problem hiding this comment.
So, in that case could you please make the change and merge this PR? :)
There was a problem hiding this comment.
@AgrimPrasad I'm waiting for an approval from @vhmth :)
|
Seal of approval
|
|
@vhmth I think it's a good conversation to have. Great points all around. Also, I prefer my seals like this: |
Web Extension support is broken because Firefox does not support
externalnamespace messaging.Added checks in
wrapStoreto make sure the external listeners areactually defined before adding.
Fixes #54