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

fix: return error instead of throw #2074

Merged
merged 1 commit into from
Apr 11, 2022
Merged

fix: return error instead of throw #2074

merged 1 commit into from
Apr 11, 2022

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Mar 25, 2022

Fixes #2069

License: MIT
Signed-off-by: Henrique Dias hacdias@gmail.com

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
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.

Could be me not understanding how to test this, but it does not seem to work as expected.

If I create .ipfs/api with /ip4/127.0.0.1/tcp/5006 and then start npx serve -l 5006 to simulate non-HTTP service on that port, and then try to start ipfs-desktop, I see no error.

Electron window with ipfs-webui is displayed in broken state and [ipfsd] start daemon HTTPError: is logged to the console, but no dialog is displayed to the user.

@hacdias
Copy link
Member Author

hacdias commented Apr 1, 2022

@lidel can you try on main? Because that also doesn't trigger an error on main for me. The problem seems to be more profound. We need to display a nice dialog when there is an error starting the daemon. The problem right now is that different errors are handled in different ways.

The original issue (#2069) is that the user is running something on IPFS Gateway / API port already. In that case, IPFS Desktop will ask you to change a port or not. If you decided not to change, IPFS Desktop would close even though we handled the situation gracefully. It's not about having a remote API.

We have to see if what is missing here is a dialog saying that IPFS failed to start, or keep just letting IPFS Desktop fail with the default dialog.

@hacdias
Copy link
Member Author

hacdias commented Apr 8, 2022

Ping @lidel

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.

For some reason, I was under the impression this PR introduced regression around API port, but if the behavior was the same in main then fine merge this. API issues are separate things, need separate issues/PRs if they are a real problem (seems no?)

@lidel lidel merged commit d389733 into main Apr 11, 2022
@lidel lidel deleted the fix/do-not-throw-ports branch April 11, 2022 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[gui error report] Error: ports already being used
2 participants