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.

Sign up for GitHub

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

Jump to bottom

Fix startup problems (fix for #1417) #1419

Merged
merged 2 commits into from
Jun 10, 2018
Merged

Fix startup problems (fix for #1417) #1419

merged 2 commits into from
Jun 10, 2018

Conversation

Borewit
Copy link
Member

@Borewit Borewit commented Jun 7, 2018

Reference: issue: #1417

A promise resolve result ended up as an err argument in the callback, causing initialization of application data to fail.

@Borewit Borewit self-assigned this Jun 7, 2018
@Borewit Borewit added the bug label Jun 7, 2018
@dbrowne24
Copy link

@Borewit thanks :] appreciate you fixing it. I thought I must be doing something stupid. I'll edit my code manually until it gets merged.

@Borewit
Copy link
Member Author

Borewit commented Jun 8, 2018

No problem @dbrowne24

Alternatively you can checkout the fixed branch directly:

git remote add borewit https://github.com/Borewit/webtorrent-desktop.git
git fetch borewit
git checkout fix/issue-1417

@Borewit
#1417: Simplify code
5cdb3d0 
- use forEach instead of map (because return value was ignored)
- use Promise.all instead of parallel, because tasks are already Promise based. Fewer callbacks.
DiegoRBaquero
DiegoRBaquero approved these changes Jun 9, 2018
View reviewed changes
Copy link
Member

@DiegoRBaquero DiegoRBaquero left a comment

Choose a reason for hiding this comment

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

Great, we also get rid of a dep using promise all

@Borewit
Copy link
Member Author

Borewit commented Jun 10, 2018

@feross please review with priority.

@codealchemist
Copy link
Contributor

Able to reproduce on master:

 ~/dev/webtorrent-desktop/ [master] npm start

> [email protected] start /Users/bert/dev/webtorrent-desktop
> npm run build && electron .


> [email protected] build /Users/bert/dev/webtorrent-desktop
> buble src --output build

Missing config file: Creating new one
(node:37622) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 1): ,,
(node:37622) DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
(node:37622) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): ,,
(node:37622) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 3): ,,
(node:37622) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 4): ,,
(node:37622) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 5): ,,
(node:37622) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 6): ,,
(node:37622) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 7): ,,
(node:37622) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 8): ,,
(node:37622) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 9): ,,
(node:37622) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 10): ,,

codealchemist
codealchemist approved these changes Jun 10, 2018
View reviewed changes
Copy link
Contributor

@codealchemist codealchemist left a comment

Choose a reason for hiding this comment

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

Working as expected on fix branch.
image

Good work guys!
Thanks!

@codealchemist codealchemist merged commit a1cfe87 into webtorrent:master Jun 10, 2018
@Borewit Borewit deleted the fix/issue-1417 branch June 11, 2018 09:20
@Borewit
Copy link
Member Author

Borewit commented Jun 11, 2018

Thanks for reviewing guys.

@codealchemist codealchemist mentioned this pull request Jun 15, 2018
Closed
@lock lock bot locked as resolved and limited conversation to collaborators Sep 9, 2018
feross
feross reviewed Sep 7, 2019
View reviewed changes
src/renderer/lib/state.js
})
Promise.all(tasks)
.then(() => cb(null, saved))
.catch(err => cb(err))
Copy link
Member

Choose a reason for hiding this comment

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

This is not the correct way to call an error callback with a promise. It's an easy mistake to make.

The .catch will catch more than just the promise rejections from the tasks array. It will also catch any exceptions thrown from the handler in .then() which means that if the cb function throws an exception, then we'll catch it here.

Instead we should use .then(success, fail). I'll send a PR.

Copy link
Member

Choose a reason for hiding this comment

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

PR here: #1679

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@feross feross feross left review comments

@DiegoRBaquero DiegoRBaquero DiegoRBaquero approved these changes

@codealchemist codealchemist codealchemist approved these changes

@dcposch dcposch Awaiting requested review from dcposch

Assignees

@Borewit Borewit

Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Borewit @dbrowne24 @codealchemist @feross @DiegoRBaquero