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.

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

Standard way for renderer extensions to display errors #4465

Merged
merged 6 commits into from May 15, 2018

Conversation

@gnestor
Copy link
Contributor

@gnestor gnestor commented Apr 25, 2018

Closes #4347

output.renderModel(model);
output.renderModel(model).catch(error => {
// Add stderr message to output
const stderr = `Javascript Error: ${error.message}. This usually means there's a typo in your chart specification. See the JavaScript console for the full traceback.`;
Copy link
Member

@ian-r-rose ian-r-rose Apr 25, 2018

This error message is not particulary generic, since output types don't necessarily have a chart spec.

Copy link
Contributor Author

@gnestor gnestor Apr 26, 2018

Good catch. That's leftover from vega3-extension-specific error message 😬

Copy link
Contributor Author

@gnestor gnestor Apr 26, 2018

This approach is pretty hacky BTW. I tried creating a new widget using this.rendermime.createRenderer with the application/vnd.jupyter.stderr mime type and replacing the current widget with it (which would be much less hacky) but it doesn't work because return widget below happens before this promise ever catches.

@vidartf
Copy link
Member

@vidartf vidartf commented Apr 26, 2018

Won't this cause the frontend error output to be saved back to disk on save? If so, it would overwrite the actual output? Or am I misunderstanding how this works?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 26, 2018

Won't this cause the frontend error output to be saved back to disk on save? If so, it would overwrite the actual output?

When I glance at it, that's how I see it working too - I hope I'm misunderstanding it. I'm strongly opposed to changing the nb model that gets saved back to disk based on rendering errors in the JupyterLab.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Apr 26, 2018

That's right, the model.setData line should change the output area model to the error. I think that's mostly an orthogonal question to having a standardized display for rendering errors: we could remove that line without changing the behavior of the error displays.

@vidartf
Copy link
Member

@vidartf vidartf commented Apr 27, 2018

I think this version makes sense. @jasongrout : With this change, what will happen for widgets if a module/model is not found? Will it show the 'text/plain' data, or this error output? Which behavior do we want?

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Apr 27, 2018

AFAIK, ipywidgets returns a Python error of application/vnd.jupyter.stderr mime type which this PR is trying to emulate for JS/client-side errors.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 27, 2018

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 27, 2018

(in other words, ipywidgets probably won't trigger this code since it catches its own rendering errors and puts an error message in itself)

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Apr 27, 2018

But it could be updated to use this code if that was the standard expectation for mimerenderers.

@vidartf
Copy link
Member

@vidartf vidartf commented Apr 28, 2018

But it could be updated to use this code if that was the standard expectation for mimerenderers.

Maybe that would be a more informative error? And it would also include the exception directly on screen to the user, so that we don't need to ask users to go into the console and report back any errors every time 😄.

Sorry for taking the discussion off-topic though.

@gnestor gnestor force-pushed the mimerender-errors branch from b24e795 to 848677a May 3, 2018
@gnestor
Copy link
Contributor Author

@gnestor gnestor commented May 3, 2018

@vidartf @jasongrout Yes, if ipywidgets just rejects the promise with the error (Promise.reject(err)), then a) ipywidgets wouldn't need to worry about rendering error messages and b) the error messages would be much more descriptive. We could iterate on this and actually include stacktraces, links to source files (that could open them in lab)...something along the lines of https://github.com/commissure/redbox-react. For now though, the plain text error messages are consistent with Python errors.

@jasongrout Care to merge this one?

// Manually append error message to output
output.node.innerHTML = `<pre>Javascript Error: ${error.message}</pre>`;
// Remove mime-type-specific CSS classes
(output.node.classList as any).forEach((className: string) => {
Copy link
Contributor

@jasongrout jasongrout May 4, 2018

It would be simpler and less brittle to just snapshot the className before calling the renderer, and then just reset it here.

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented May 9, 2018

@jasongrout I think this is ready 👍

@gnestor gnestor added this to the Beta 3 milestone May 9, 2018
Copy link
Contributor

@ellisonbg ellisonbg left a comment

We looked at this in a dev call and it looks good. Thanks!

@ellisonbg ellisonbg merged commit 29fd451 into jupyterlab:master May 15, 2018
2 checks passed
gnestor added a commit to gnestor/jupyterlab that referenced this issue May 21, 2018
gnestor added a commit to gnestor/jupyterlab that referenced this issue May 21, 2018
gnestor added a commit to gnestor/jupyterlab that referenced this issue May 21, 2018
@gnestor gnestor mentioned this pull request May 29, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants