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

Replace request with axios #516

Open
wants to merge 5 commits into
base: next
Choose a base branch
from
Open

Conversation

r0wanda
Copy link

@r0wanda r0wanda commented Dec 7, 2024

Request is deprecated (request/request#3142), and axios is already part of this project's dependencies, so it feels more appropriate to stick to one http library. I tried to maintain the stream approach in the original request-based code.

@Mastermindzh Mastermindzh changed the base branch from master to next December 9, 2024 10:16
@Mastermindzh
Copy link
Owner

Did you test this :) ?

Axios shouldn't be available in the renderer (and that calls downloadfile atm).

@r0wanda
Copy link
Author

r0wanda commented Dec 10, 2024

I did test it ;)
Also, why wouldn't axios be available in the renderer process? It looks like the only call to downloadfile is in the preload script, which isn't sandboxed. Sorry if this is an obvious question, I'm not very experienced with electron.

@Mastermindzh
Copy link
Owner

@Mar0xy, could you try testing it as well? I can't get @r0wanda's repo to work for me. Notifications are always shown without an icon.

@r0wanda, are you sure it's triggering? Can you add a Logger.log in both the success/catch blocks?
I've had this issue before with Axios not working when called directly from the renderer (which is what we're doing).

I don't know why it doesn't work, I've only ever managed to get it to work with request.

@Mastermindzh
Copy link
Owner

My testing gives me this:

image

I added the following log statements:
image
image

Copy link

sonarcloud bot commented Dec 13, 2024

@r0wanda
Copy link
Author

r0wanda commented Dec 13, 2024

I redesigned it a bit so that Axios is called in the main process through IPC. I tested it (more thoroughly this time) and it seems to work :)

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