FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(image): uncaught error in promise with image handling by NathanWalker · Pull Request #6453 · NativeScript/NativeScript · GitHub

/ NativeScript Public

fix(image): uncaught error in promise with image handling - #6453

Merged
vakrilov merged 3 commits into
NativeScript:masterfrom
NathanWalker:fix-uncaught-promise-on-image
Nov 13, 2018
ConversationCommits (3)ChecksFiles changed
Merged

fix(image): uncaught error in promise with image handling#6453
vakrilov merged 3 commits into
NativeScript:masterfrom
NathanWalker:fix-uncaught-promise-on-image

Conversation

NathanWalker commented Oct 24, 2018

Copy link
Copy Markdown
Contributor

PR Checklist

What is the current behavior?

Apps can error if url is invalid or response content is invalid.

What is the new behavior?

Error is properly caught and rejected to avoid Error: Uncaught (in promise) errors.

ghost added the ♥ community PR label Oct 24, 2018

vakrilov commented Nov 2, 2018

Copy link
Copy Markdown
Contributor

Hi and thanks for the PR!

The team have been quite busy baking the upcoming NativeScript 5.0 release. We hope you love it as much as we do! The major release is the reason we taking longer with PR reviews. We will resume active reviewing in the following weeks.

Thanks for your patience and for being awesome contributor!

vakrilov suggested changes Nov 12, 2018
View reviewed changes
Comment thread tns-core-modules/http/http-request/http-request.ios.ts Outdated
return new Promise<http.HttpResponse>((resolve, reject) => {

if (!options.url) {
reject('Request url was empty.');

vakrilov Nov 12, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Can you reject with an error (reject(new Error('Request url was empty.'));) to be consistent with the other reject path.

NathanWalker Nov 13, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good call 👍 Updated now.

vakrilov approved these changes Nov 12, 2018
View reviewed changes

vakrilov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Approve - once the comment is addressed.

NathanWalker force-pushed the fix-uncaught-promise-on-image branch from b625cf0 to f7ee2bd Compare November 13, 2018 03:39
ghost assigned vakrilov Nov 13, 2018
ghost added in progress and removed ♥ community PR labels Nov 13, 2018

vakrilov commented Nov 13, 2018

Copy link
Copy Markdown
Contributor

test

vakrilov merged commit 950fdcf into NativeScript:master Nov 13, 2018
ghost removed the in progress label Nov 13, 2018

lock Bot commented Nov 13, 2019

Copy link
Copy Markdown

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

lock Bot locked and limited conversation to collaborators Nov 13, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
vakrilov vakrilov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

vakrilov

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL