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

[Bug] Expressions Service: Render swallows rendering errors #51153

Closed
Dosant opened this issue Nov 20, 2019 · 1 comment
Closed

[Bug] Expressions Service: Render swallows rendering errors #51153

Dosant opened this issue Nov 20, 2019 · 1 comment
Assignees
@Dosant
Labels
bug Fixes for quality problems that affect the customer experience v7.6.0

Comments

@Dosant
Copy link
Contributor

Dosant commented Nov 20, 2019 •
edited
Loading

Kibana version: 7.6

Describe the bug:

There is no default error handling inside

kibana/src/plugins/expressions/public/render.ts

Line 78 in ff2a3ab

render = (data: Data, extraHandlers: IExpressionRendererExtraHandlers = {}) => {

So visualisations which are using it directly might be missing error handling (previously it was notification service snackbar).

Need to rethink error handling API of expression service to be able to use notification service by default from within render.ts

Reproduce:

  1. Go to vertical bar chart visualisation
  2. In advanced settings put valid json, e.g. {"a":false}
  3. Nothing happens

Expected:
In 7.5.0 there is an error:
Screenshot 2019-11-20 at 13 32 20
@Dosant Dosant added bug Fixes for quality problems that affect the customer experience Team:AppArch v7.6.0 labels Nov 20, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@Dosant Dosant self-assigned this Nov 20, 2019
@Dosant Dosant mentioned this issue Nov 20, 2019
Merged
3 tasks
Dosant added a commit to Dosant/kibana that referenced this issue Nov 21, 2019
@Dosant
fix expressions service render swallows rendering errors elastic#51153
88a753a 
prettier
Dosant added a commit to Dosant/kibana that referenced this issue Nov 22, 2019
@Dosant
fix expressions service render swallows rendering errors elastic#51153
3655120 
prettier

remove error$ and errorRenderer, use onRenderError callback instead for both

use custom error handler only if it is provided by consumer, otherwise fallback to default handler

add comment

fix linter issues

fix
Dosant added a commit that referenced this issue Nov 27, 2019
@Dosant
Expressions service fixes: better error and loading states handling (#…
0fd63ee 
…51183)

1. This pr fixes regression in v7.6 - #51153.

Visualisation which are using ExpressionLoader directly are swallowing render errors in 7.6, because generic error handling is happening only on expression_renderer.tsx level (react component wrapper around renderer).

Now react agnostic render code render.ts, loader.ts:

has it's own default error handler which is toast from notification service.
It also allows to pass custom error handler instead of default one
React expression renderer expression_renderer.tsx:

if consumer wants to render error as react component inline, then the component uses custom error handler from render.ts to wire up react component.

2. This pr fixes issue of loader.ts where initial loading$ was not emitted

Had to change loadingSubject from Subject to BehaviorSubject to remember the last value

3. This pr fixes dependencies in effects of expression_renderer.tsx
Dosant added a commit to Dosant/kibana that referenced this issue Nov 27, 2019
@Dosant
Expressions service fixes: better error and loading states handling (e…
bbcebe0 
…lastic#51183)

1. This pr fixes regression in v7.6 - elastic#51153.

Visualisation which are using ExpressionLoader directly are swallowing render errors in 7.6, because generic error handling is happening only on expression_renderer.tsx level (react component wrapper around renderer).

Now react agnostic render code render.ts, loader.ts:

has it's own default error handler which is toast from notification service.
It also allows to pass custom error handler instead of default one
React expression renderer expression_renderer.tsx:

if consumer wants to render error as react component inline, then the component uses custom error handler from render.ts to wire up react component.

2. This pr fixes issue of loader.ts where initial loading$ was not emitted

Had to change loadingSubject from Subject to BehaviorSubject to remember the last value

3. This pr fixes dependencies in effects of expression_renderer.tsx
# Conflicts:
#	src/plugins/expressions/public/loader.test.ts
Dosant added a commit that referenced this issue Nov 27, 2019
@Dosant
Expressions service fixes: better error and loading states handling (#…
81a1f56 
…51183) (#51800)

1. This pr fixes regression in v7.6 - #51153.

Visualisation which are using ExpressionLoader directly are swallowing render errors in 7.6, because generic error handling is happening only on expression_renderer.tsx level (react component wrapper around renderer).

Now react agnostic render code render.ts, loader.ts:

has it's own default error handler which is toast from notification service.
It also allows to pass custom error handler instead of default one
React expression renderer expression_renderer.tsx:

if consumer wants to render error as react component inline, then the component uses custom error handler from render.ts to wire up react component.

2. This pr fixes issue of loader.ts where initial loading$ was not emitted

Had to change loadingSubject from Subject to BehaviorSubject to remember the last value

3. This pr fixes dependencies in effects of expression_renderer.tsx
# Conflicts:
#	src/plugins/expressions/public/loader.test.ts
@Dosant Dosant closed this as completed Nov 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Dosant Dosant

Labels
bug Fixes for quality problems that affect the customer experience v7.6.0
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Dosant @elasticmachine