Update to new renderer API and update webpack from 4 => 5 #46
Conversation
| @@ -2,10 +2,10 @@ | |||
| // Licensed under the MIT License. | |||
|
|
|||
| const common = require('./common'); | |||
There was a problem hiding this comment.
This had a loading error and from the docs didn't look relevant to ES6 module, so I removed:
https://www.npmjs.com/package/webpack-fix-default-import-plugin
| @@ -18,21 +18,25 @@ module.exports = { | |||
| output: { | |||
| path: path.join(constants.ExtensionRootDir, 'out', 'client_renderer'), | |||
| filename: '[name].js', | |||
| chunkFilename: `[name].bundle.js` | |||
| chunkFilename: `[name].bundle.js`, | |||
| libraryTarget: 'module' | |||
There was a problem hiding this comment.
This is the change we needed webpack 5 for. Import loading now for notebook renderers:
https://github.com/microsoft/vscode-github-issue-notebooks/blob/e47c04e301025f4b024b70abadb48c73e7b13374/webpack.config.js#L72
| experiments: { | ||
| outputModule: true | ||
| }, | ||
| target: 'node', |
There was a problem hiding this comment.
Webpack 5 doesn't shim build in node modules. Per the warning message here it seems like we could either reference the shim package and add a fallback or if we target 'node' I don't believe that it's needed. Was a tiny bit unsure on this, but this seemed to work with the node loaded, so went with this for now versus referencing shim packages.
Here is an example of the type of error we would see after the 4-5 update.
https://stackoverflow.com/questions/64402821/module-not-found-error-cant-resolve-util-in-webpack
| new ForkTsCheckerWebpackPlugin({ | ||
| checkSyntacticErrors: true, | ||
| tsconfig: configFileName, | ||
| reportFiles: ['src/client/**/*.{ts,tsx}'], | ||
| memoryLimit: 9096 | ||
| }), | ||
| new DefinePlugin({ |
There was a problem hiding this comment.
Glad that Conner called this out this change specifically in the issue, would have bit me otherwise.
| output: OldNotebookOutput; | ||
| mimeType: string; | ||
| } | ||
| export const activate: ActivationFunction = () => ({ |
There was a problem hiding this comment.
Actual code change was super simple. Webpack was the only real pain. At one point we used to maintain back compat on this extension, hence the old API classes here. But I just don't see that being very possible with the new changes, so explicitly moving to 1.57 and removing the old BC code path.
| return { | ||
| data: { | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| [cellInfo.mime]: cellInfo.value as any |
There was a problem hiding this comment.
The any types are a bit ugly here, but it's actually going to those super loose Jupyter JSONObjects and I didn't want to import phosphor and whatnot for something that didn't really give any type safety. So went with any for now, but not adverse to looking if we can make these stronger types in a follow up.
IanMatthewHuff
requested review from
DonJayamanne,
joyceerhl,
DavidKutu and
rchiodo
|
Note: Build all looks fine here: https://dev.azure.com/ms/vscode-notebook-renderers/_build/results?buildId=169252&view=results Some results don't seem to be linked here though. So doing an admin push. Will check on why the CI doesn't seem tied to the github status reporting. |
microsoft/vscode#93265 (comment)
This change required webpack as ES6 module, which required moving from webpack 4 => 5.
I'm a webpack noob, so I'll call out in comments some of the changes that I made. I was seeing one issue with plotly that I want to look into, looks like it wasn't loading the plotly bundle correctly. But considering that I also need to do this for Jupyter (which might be harder) I'd rather get both extensions up and generally running before digging into issues, since this change is expected to be breaking Friday or Monday.
While I saw a plotly issue the general transforms were working. I'll test more output types as I go.