-
Notifications
You must be signed in to change notification settings - Fork 325
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
Refactor ipfs-companion so it no longer sets properties on the window #321
Conversation
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.
@alanshaw Looks good, I see how this will make writing tests easier 👍
The only thing blocking merge is a non-invasive error logged on initial load.
It is the same in both Chrome:
and Firefox:
It does not impact functionality, the contextMenuUploadToIpfs
menu item is eventually initialized and works fine, but the error should be somehow addressed.
@lidel ok all fixed up - I moved the context menu initialisation and update stuff into the |
This is quickly going to get difficult to merge with #320 and friends, so I'm gonna take a look at it now and see if it's good to go 🚀✨ 👀 |
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.
Looks good, test pass, and works for me in Firefox and Chrome. 👍 ✨
@lidel do you have a minute to merge this? Merging is blocked on re-review. In retesting this I can see the error message has been resolved. |
@olizilla my apologies, I run out of time to review this yesterday. Old error is gone, but now I get this one on every API poll (while external node is down):
Are you able to reproduce? If not, it may be the machine I am on, I'll retest on another one in 5h. |
Actually, I see the problem... pesky hoisting / lexical scoping. Am fix. |
Thanks! 👌 |
This PR refactors the
ipfs-companion
module so it no longer sets properties on the window object.I'm viewing the
ipfs-companion
module as something I'd like to create an instance of, or multiple instances of, and also teardown - with the end goal of making it easier to reason about, maintain and test. So this PR stops it from setting properties on the window object and insteadinit
returns an API object which is the method one should communicate with the module.background.js
, after creating the new instance, sets it towindow.ipfsCompanion
so that the popup, quick upload and options pages can make use of it's API.port.postMessage
to signal to the companion to perform a copy (instead of calling the functions on the background page)FileReader
logic into async/await style