Refactor showModal to avoid having catch everywhere

[arikfr]

(follow up to this conversation)

redash/client/app/components/tags-control/TagsControl.jsx

Lines 27 to 30 in 2755e12

	EditTagsDialog.showModal({ tags, getAvailableTags })
	.result.then(this.props.onEdit)
	.catch(() => {}); // ignore dismiss
	};

@arikfr:

Coudln't we handle this in showModal instead of having to add this everywhere?

@kravets-levko

Sadly, no After .catch() handler promise will go to next .then() handler, so if we catch that promises inside of dialog wrapper - the caller's .catch() will not be called. It actually is not a problem, but browser will show unhandled rejection errors in console. We can either:

1. don't catch that promises and ignore errors in console;
2. always add empty catch handler like I did here;
3. replace browser's Promise with something else that does not trigger unhandled rejection errors (e.g. "bluebird") - prior to this PR we used Angular's $q for this purpose;
4. change a way we handle dialog's close/dismiss events (for me current flow looks fine, but that annoying unhandled rejections...)

[arikfr]

@kravets-levko here's another suggestion:

When the dialog is closed or dismissed always call resolve the promise, but resolve it with values that represent whether it was dismissed and what the result was.

Is this possible?

[kravets-levko]

Yes, sure - it's actually option 4 from my list. Also I posted one more comment in that thread:

I'll explore if we can suppress that error with handling unhandledrejection event if error comes from dialog wrapper

[arikfr]

I don't think the last suggestion is a good one. There just shouldn't be an error when there isn't one :)

[kravets-levko]

Rejected promise is not an error, actually. I like current flow because it clearly describes two paths - when used accepted some action (then dialog is "resolved"), and when cancelled (then dialog is "rejected"). So when you need to handle only success path - you don't have to add any ifs to your code.

[arikfr]

We could avoid using promises here and just pass a callback for dialog closed and another (optional) for dialog cancelled. This way handler stays the same and we don't have to abuse a promise.