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

update got version #101

Closed
wants to merge 3 commits into from
Closed

Conversation

santiagonoguez
Copy link
Contributor

The version of 'got' that rise-common-electron resulted in an unhandled network error when there are network disconnections or change of network status. Releases 8.x of the library fix that, so I updated. I tested manually the library change and not only the error is now properly caught, but also download process resumes when there's again connectivity.

Release 8 of GOT ( https://github.com/sindresorhus/got/releases ) says it changes the default value of useElectronNet from true to false, but when I tried to preset it in previous PR it failed on launcher integration and manual tests. Leaving the code unchanged seems to work though, so I'm submitting this PR instead without those changes.

Copy link
Member

@tejohnso tejohnso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can just ignore the useElectronNet breaking change. We depend on that for chromium's proxy handling.

@santiagonoguez
Copy link
Contributor Author

@tejohnso ok, I'll further investigate why my changes didn't work

@santiagonoguez
Copy link
Contributor Author

@tejohnso Finally found why I wasn't able to make it work in launcher. Unfortunately there's an open bug with GOT >8 where useElectronNet causes an error to be thrown. This was reported last December:
sindresorhus/got#443

And that bugs in turn depends upon an open bug in electron. This has been reported on December 2016 and still not fixed:
electron/electron#8117

Also unfortunately, the fix we were looking for the installer crash is not available before that version. Thoughts ?
@stulees @andrecardoso

@santiagonoguez
Copy link
Contributor Author

As agreed, I'll work in a workaround directly in launcher with GOT 7 for the time being, and will reopen this PR later with a new GOT version when this particular bug is solved.

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