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

first stab at putting protocol handlers in all processes #28

Merged
merged 1 commit into from
Dec 17, 2015

Conversation

the8472
Copy link
Contributor

@the8472 the8472 commented Dec 15, 2015

it's not quite done, but i'd like some feedback. i had to untangle the gui and configuration concerns a bit to make it work in the child process.

@lidel
Copy link
Member

lidel commented Dec 15, 2015

Thank you. Went over it and I am okay with the direction of your changes.
Use of supressRecursion feels bit 'dirty', but I understand why it is there.

Some cosmetic things to keep in mind:

Let me know when you feel it is ready ( if we merge this first, #29 will need to rebase :-) )

@the8472
Copy link
Contributor Author

the8472 commented Dec 15, 2015

All tests pass now. The last jshint thing seems bogus to me since function definitions are hoisted while consts have a temporal dead zone.

@lidel
Copy link
Member

lidel commented Dec 16, 2015

I agree, in this particular case moving function does not improve readability.
To whitelist, just add this comment:

function toggle(val) { /*jshint ignore:line*/

@the8472
Copy link
Contributor Author

the8472 commented Dec 17, 2015

all green now

lidel added a commit that referenced this pull request Dec 17, 2015
e10s: putting protocol handlers in all processes
@lidel lidel merged commit 20d43b6 into ipfs:master Dec 17, 2015
@lidel
Copy link
Member

lidel commented Dec 17, 2015

👍

@lidel
Copy link
Member

lidel commented Dec 20, 2015

@the8472 could you please take a look at #33 ?

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.

2 participants