Update for new NotebookCellOutput changes #35
Conversation
|
Note I have more testing to do here. But wanted to put this up earlier rather than later since I've not worked in this area much. Would rather get feedback if I'm barking up any wrong trees earlier. Main testing is to update the downloaded renderer bits alongside VS Code stable to make sure that the old API path still works. Also there needs to be update changes in the ipywidget code in the jupyter extension. Will file separate issue and handle that separate. |
| data: { | ||
| [request.mime]: request.value | ||
| }, | ||
| metadata: request.metadata || {}, |
There was a problem hiding this comment.
Metadata is the bit that I want to check in particular when I look at the old API with stable. The new layout is a bit odd. There is a top-level metadata and then another unlabeled metadata just below that. I think it's the old custom metadata as it has needs_background in it.
There was a problem hiding this comment.
Mostly the naming seems odd. metadata.metadata feels vague to me.
There was a problem hiding this comment.
Should we give VS Code that feedback?
IanMatthewHuff
requested review from
DonJayamanne,
DavidKutu,
joyceerhl and
rchiodo
|
Testing done:
@DonJayamanne Any other particular areas of testing that I should do to verify this? |
| try { | ||
| console.error('request', request); | ||
| console.log('request', request); |
There was a problem hiding this comment.
Not sure why these were errors, so I converted them.
| if (outputMetadata && outputMetadata[request.mimeType] && outputMetadata[request.mimeType].metadata) { | ||
| const metadata: Record<string, any> = {}; | ||
|
|
||
| const oldRequest = (request as unknown) as OldNotebookOutputEventParams; |
There was a problem hiding this comment.
Is casting to unknown better practice?
There was a problem hiding this comment.
That was what I was reading. (Full confession, I didn't actually know this beforehand, and looked it up after a hint from a TS compiler warning). Here is Anders commenting on the addition of unknown. Basically a typesafe any so that you can use it safely as an intermediate with casting.
microsoft/TypeScript#24439
| data: { | ||
| [request.mime]: request.value | ||
| }, | ||
| metadata: request.metadata || {}, |
There was a problem hiding this comment.
Should we give VS Code that feedback?
|
I think the build is working, but it seems disconnected from the status page here. Will investigate: |
|
@IanMatthewHuff you might want to update the code in Jpuyter extension, we have some code related to ipywidgets as well. src/datascience-ui/ipywidgets/renderer/index.ts |
|
Squashing as tests are passing on AzDO |
Update per changes here:
microsoft/vscode#117444