Skip to content

Conversation

rchiodo
Copy link

@rchiodo rchiodo commented Apr 1, 2020

For #9523

When loading 3rd party controls, their source may not be on unpkg.com. When this happens show an error to the user. If they're not connected to the internet, tell them this is likely why.

I tested this by removing our require define for azureml (as it's not on unpkg.com)

@codecov-io
Copy link

Codecov Report

Merging #10907 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10907      +/-   ##
==========================================
- Coverage   60.67%   60.65%   -0.02%     
==========================================
  Files         580      580              
  Lines       31530    31531       +1     
  Branches     4480     4480              
==========================================
- Hits        19131    19126       -5     
- Misses      11429    11433       +4     
- Partials      970      972       +2     
Impacted Files Coverage Δ
...ience/interactive-common/interactiveWindowTypes.ts 100.00% <ø> (ø)
src/client/datascience/jupyter/jupyterNotebook.ts 4.27% <ø> (ø)
src/client/datascience/jupyter/jupyterSession.ts 56.80% <ø> (ø)
src/client/datascience/types.ts 100.00% <ø> (ø)
src/client/common/utils/localize.ts 95.20% <100.00%> (+0.01%) ⬆️
src/datascience-ui/react-common/arePathsSame.ts 75.00% <0.00%> (-12.50%) ⬇️
src/client/common/utils/platform.ts 64.70% <0.00%> (-11.77%) ⬇️
src/client/linters/pydocstyle.ts 86.66% <0.00%> (-2.23%) ⬇️
src/client/datascience/debugLocationTracker.ts 76.56% <0.00%> (-1.57%) ⬇️
src/client/common/process/proc.ts 14.49% <0.00%> (-0.73%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c523d6b...a95a8af. Read the comment docs.

@rchiodo rchiodo self-assigned this Apr 1, 2020
@@ -201,4 +201,47 @@ export namespace CommonEffects {
cellVMs: newVMs
};
}

export function handleLoadIPyWidgetClassFailure(

Choose a reason for hiding this comment

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

Should we add some telemetry for this.
Would be good to track how often this happens and whether it is due to lack of network connection.

Copy link
Author

Choose a reason for hiding this comment

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

Good idea. Will do.

@@ -579,12 +579,6 @@ export class CellOutput extends React.Component<ICellOutputProps> {
// Silently skip rendering of these mime types, render an empty div so the user sees the cell was executed.
buffer.push(<div key={index}></div>);
} else {
if (transformed.output.data) {
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, what's this removal for?

Copy link
Author

Choose a reason for hiding this comment

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

There's a bug here. This used to go through this case when the mimetype was empty. But it goes through this case now when we don't have a match (mimetype empty does nothing). The logic here is unnecessary if we have an actual mimetype.

This repros with the azureml widget as it puts up a 'loading' widget that we don't support.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Apr 1, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@rchiodo rchiodo requested a review from DonJayamanne April 1, 2020 22:11
@DonJayamanne
Copy link

@rchiodo
Thanks for cleaning up the imports by using import type, better to be explicit

@rchiodo rchiodo merged commit 14d81c8 into master Apr 1, 2020
@rchiodo rchiodo deleted the rchiodo/load_widget_failure branch April 1, 2020 23:32
@lock lock bot locked as resolved and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants