Expose fatalErrors API from the Start contract

[mshustov]

Summary

This API intended to be used for runtime exceptions. To improve DX, we should provide it as a part of the start contract as well.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

• [ ] This was checked for cross-browser compatibility, including a check against IE11
• [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• [ ] Unit or functional tests were updated or added to match the most common scenarios
• [ ] This was checked for keyboard-only and screenreader accessibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately
• This includes a feature addition or change that requires a release note and was labeled appropriately

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[pgayvallet]

LGTM, as long as we are exposing this in setup, it makes sense to do the same in start.

On an architectural point of view however, I'm wondering if exposing this API to plugin is really a good thing. Now that Kibana is going to be a 'real' SPA, does that really make sense to be able to halt the whole client-side application 'only' based on a plugin decision/error? Am I the only one that this bother, or should I create an issue to discuss this point?

[pgayvallet]

NIT/discussion: Personal opinion, but I always avoided one-liner blocks without brackets. Our linter configuration does not complains so this is not an issue, but do we have conventions on that?

[mshustov]

no lint rule === no convention. I can update anyway to avoid confusion.

[pgayvallet]

Not necessary, as I said, linter is alright with this, so no need to change imho

[mshustov]

On an architectural point of view however, I'm wondering if exposing this API to plugin is really a good thing. Now that Kibana is going to be a 'real' SPA, does that really make sense to be able to halt the whole client-side application 'only' based on a plugin decision/error? Am I the only one that this bother, or should I create an issue to discuss this point?

It's a substitution for the legacy fatalError API

kibana/src/legacy/ui/public/notify/fatal_error.ts

Line 33 in 7d01dd0

export function fatalError(error: AngularHttpError | Error | string, location?: string) {

which used mostly to render an error message for the failed operation.
I do agree it can be misused easily. OTOH I can imagine that plugins do need such API in SPA. For example, a data sync operation in the background fails, a plugin might want to prevent any future user interactions to avoid data inconsistency.
Having said that I think it's a legitimate case for a plugin to use this API, but every case should be considered separately.

[pgayvallet]

which used mostly to render an error message for the failed operation.

It does by erasing the whole dom content of the page in current implementation though.

kibana/src/core/public/fatal_errors/fatal_errors_service.tsx

Lines 109 to 127 in 812c950

	private renderError(injectedMetadata: InjectedMetadataSetup, i18n: I18nStart) {
	// delete all content in the rootDomElement
	this.rootDomElement.textContent = '';
	// create and mount a container for the <FatalErrorScreen>
	const container = document.createElement('div');
	this.rootDomElement.appendChild(container);
	render(
	<i18n.Context>
	<FatalErrorsScreen
	buildNumber={injectedMetadata.getKibanaBuildNumber()}
	kibanaVersion={injectedMetadata.getKibanaVersion()}
	errorInfo$={this.errorInfo$}
	/>
	</i18n.Context>,
	container
	);
	}

but that's outside of the scope of the PR.

[mshustov]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: ce94b04

History

• 💚 Build #21440 succeeded a5332b3
• 💚 Build #21427 succeeded 7be06dd
• 💔 Build #21355 failed cb55b83

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[streamich]

AppArch changes LGTM.

[lizozom]

LGTM

[mshustov]

@pgayvallet #55402