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: Android notification not working #3796 #3801

Closed
wants to merge 2 commits into from
Closed

Fix: Android notification not working #3796 #3801

wants to merge 2 commits into from

Conversation

InfinityLoop1308
Copy link

Fix: Android notification not working #3796
Fix: remove useless dependency full-icu, which caused problem executing npm ci (See this issue)

Note: I don't really know if this fix of #3796 is the supposed way, but it works.

@InfinityLoop1308 InfinityLoop1308 marked this pull request as ready for review January 17, 2022 13:13
@InfinityLoop1308
Copy link
Author

Another thing need to be mentioned: keytar needs libsecret so maybe a step of installing libsecret should be added to build instruction.

@charlag
Copy link
Contributor

charlag commented Jan 17, 2022

full-ice is a devDependency needed for tests, we will not remove it.
We will add a note about libsecret or at least make postinstall script not fail if it's not there.

I don't understand the fix yet (and it doesn't seem like you do either), there's already reschedule done in handleException() if it's appropriate. Did you find if you run into some branch that's failing? Can we reproduce it somehow?

@InfinityLoop1308
Copy link
Author

InfinityLoop1308 commented Jan 17, 2022

full-ice is a devDependency needed for tests, we will not remove it. We will add a note about libsecret or at least make postinstall script not fail if it's not there.

see the link I quoted - it is a built-in module for node 16 above so there is no need to include it(and it made me fail doing npm ci)
Nodejs change log

I don't understand the fix yet (and it doesn't seem like you do either), there's already reschedule done in handleException() if it's appropriate. Did you find if you run into some branch that's failing? Can we reproduce it somehow?

The problem is, there is not any exception. I have no idea why this happened.

@charlag
Copy link
Contributor

charlag commented Jan 17, 2022

Oh, sorry, I overlooked the link about icu. We will remove it but let's do it separately from Android issue.

I think we should try to find out what happens in your case first. If there's no exception then it was a "happy" path

@InfinityLoop1308
Copy link
Author

I think we should try to find out what happens in your case first. If there's no exception then it was a "happy" path

Sorry but I don't really know what is a "happy" path. Could you give more information?

@charlag
Copy link
Contributor

charlag commented Jan 19, 2022

@InfinityLoop1309 I meant that if no error has happened then it's probably working as intended - connects, waits for one response and then hangs until system says it's time to sleep again.
We need to understand the problem in order to fix it.

@InfinityLoop1308
Copy link
Author

InfinityLoop1308 commented Jan 19, 2022

@InfinityLoop1309 I meant that if no error has happened then it's probably working as intended - connects, waits for one response and then hangs until system says it's time to sleep again. We need to understand the problem in order to fix it.

@charlag Seems the only reason it ran into finally without an exception is that reader.readLine() returns null. But why this occured remains a question.

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