-
Notifications
You must be signed in to change notification settings - Fork 56
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
Add Firefox support #1154
base: main
Are you sure you want to change the base?
Add Firefox support #1154
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Macil and I would want any new MV2 specific bits to be dropped prior to accepting this PR.
- Removed `webextension-polyfill`
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@wegry brought to my intention that the injectScriptImplementation() function inside Spelling out my second suggestion a little further: we can add a packages/core/firefox.js file with this content: import {_setInjectScriptImplementation} from './inboxsdk';
_setInjectScriptImplementation(() => {
const script = document.createElement('script');
script.type = 'text/javascript';
script.src = browser.runtime.getURL('pageWorld.js');
document.documentElement.appendChild(script);
});
export * from './inboxsdk'; and a packages/core/firefox.d.ts: export * from './inboxsdk'; and we need to make sure that it's guaranteed that the injectScriptImplementation function is only called asynchronously after the InboxSDK import. I think this may already be the case because of an // Call injectScript asynchronously so InboxSDK alt imports like firefox.js can patch it first
var pageCommunicatorPromise = Promise.resolve().then(injectScript).then(() => pageCommunicator); (A cleaner solution that didn't need the asynchronous call to injectScript would be for us to not bundle the InboxSDK in our npm release, and then the firefox.js module could import a module that overrides the inject script function before importing and re-exporting the main InboxSDK entrypoint. But it's not so much cleaner that we need to jump to that now.) |
@Macil Taking your suggestion into account, I've removed all So userland usage will now work like this (firefox content script): import InboxSDK from '@inboxsdk/core/firefox' How it worksBackgroundIn the background.js script the InboxSDK/packages/core/background.js Lines 5 to 12 in 21d765d
Content scriptI've added inboxsdk-FIREFOX.ts file which listens for the event and injects pageWorld.js InboxSDK/src/inboxsdk-js/inboxsdk-FIREFOX.ts Lines 1 to 21 in 21d765d
This method eliminates the need for Additional notesBuild configI've added the firefox entry to the Lines 402 to 409 in 21d765d
Please let me know if these changes satisfy your concerns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
I think what we're waiting for prior to a merge is @Macil will sign off on this as well. |
If see it right, then https://bugzilla.mozilla.org/show_bug.cgi?id=1736575 looks like has been resolved, so this could be iterated upon in order not to require an additional import, correct? |
Is there any update about this PR? |
resolves #1105
This PR adds support for Firefox.
Userland usage
Background script
Content script
Additional Notes
This additional import is mandatory until firefox fixes MAIN world execution in content scripts (See here).