Perf improvements to the ipynb serializer #131035
Conversation
| @@ -61,8 +69,19 @@ function sortOutputItemsBasedOnDisplayOrder(outputItems: NotebookCellOutputItem[ | |||
| } | |||
| return compareWith.startsWith(value); | |||
| }; | |||
| const indexOfMimeTypeA = orderOfMimeTypes.findIndex(mime => isMimeTypeMatch(outputItemA.mime, mime)); | |||
| const indexOfMimeTypeB = orderOfMimeTypes.findIndex(mime => isMimeTypeMatch(outputItemB.mime, mime)); | |||
| let indexOfMimeTypeA = orderOfMimeTypes.findIndex((mime) => isMimeTypeMatch(mime, outputItemA.mime)); | |||
There was a problem hiding this comment.
Added something that was fixed in Jupyter extension (some how this was missing here).
I've added the changes/fixes here along with the corresponding tests.
| @@ -94,7 +94,7 @@ function translateCellDisplayOutput(output: NotebookCellOutput): JupyterOutput { | |||
| case 'display_data': { | |||
| result = { | |||
| output_type: 'display_data', | |||
| data: output.items.reduceRight((prev: any, curr) => { | |||
There was a problem hiding this comment.
Not sure why it was reduceRight, it should be reduce (fix everywhere else)
| .reduceRight<string[]>((prev, curr) => prev.concat(curr), []); | ||
| .forEach(value => { | ||
| // Ensure each line is a seprate entry in an array (ending with \n). | ||
| const lines = value.split('\n'); |
There was a problem hiding this comment.
Made the changes as per the comments here #129370 (comment)
There was a problem hiding this comment.
for the split, we do want to handle CRLF?
There was a problem hiding this comment.
I don't think we need to (this is how we used to do it & based on what I know this is what Jupyter also does, - code was borrowed from Jupyter)
|
|
||
| suite('Output Order', () => { | ||
| test('Verify order of outputs', async () => { | ||
| const dataAndExpectedOrder: { output: nbformat.IDisplayData; expectedMimeTypesOrder: string[] }[] = [ |
There was a problem hiding this comment.
Tests for a fix that was missing
| .reduceRight<string[]>((prev, curr) => prev.concat(curr), []); | ||
| .forEach(value => { | ||
| // Ensure each line is a seprate entry in an array (ending with \n). | ||
| const lines = value.split('\n'); |
There was a problem hiding this comment.
for the split, we do want to handle CRLF?
This PR fixes #129370