Skip to content
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

Merged
merged 6 commits into from
Dec 1, 2017

Conversation

alanshaw
Copy link
Member

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 instead init returns an API object which is the method one should communicate with the module.

background.js, after creating the new instance, sets it to window.ipfsCompanion so that the popup, quick upload and options pages can make use of it's API.

  • Pulls out the notifier logic into it's own module
  • Pulls out the copying logic into it's own module
  • Uses port.postMessage to signal to the companion to perform a copy (instead of calling the functions on the background page)
  • Refactors some of the async FileReader logic into async/await style

Copy link
Member

@lidel lidel left a 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:

2017-11-30-000250_605x83_scrot

and Firefox:

2017-11-30-000303_642x155_scrot

It does not impact functionality, the contextMenuUploadToIpfs menu item is eventually initialized and works fine, but the error should be somehow addressed.

@alanshaw
Copy link
Member Author

@lidel ok all fixed up - I moved the context menu initialisation and update stuff into the context-menus module and that has fixed it.

@olizilla
Copy link
Member

olizilla commented Dec 1, 2017

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 🚀✨ 👀

Copy link
Member

@olizilla olizilla left a 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. 👍 ✨

@olizilla
Copy link
Member

olizilla commented Dec 1, 2017

@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 olizilla mentioned this pull request Dec 1, 2017
@lidel
Copy link
Member

lidel commented Dec 1, 2017

@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):

ReferenceError: can't access lexical declaration `offlinePeerCount' before initialization  background.js:360:

Are you able to reproduce? If not, it may be the machine I am on, I'll retest on another one in 5h.

@olizilla
Copy link
Member

olizilla commented Dec 1, 2017

@lidel yes, good spot, I'm seeing that too. @alanshaw can you take a look when you get a moment?

@olizilla
Copy link
Member

olizilla commented Dec 1, 2017

Actually, I see the problem... pesky hoisting / lexical scoping. Am fix.

@lidel lidel merged commit e3a593e into ipfs:master Dec 1, 2017
@lidel
Copy link
Member

lidel commented Dec 1, 2017

Thanks! 👌

@alanshaw alanshaw deleted the remove-bg-page-api branch December 2, 2017 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants