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

Add require to preload #6318

Merged
merged 3 commits into from
Jun 22, 2021
Merged

Add require to preload #6318

merged 3 commits into from
Jun 22, 2021

Conversation

IanMatthewHuff
Copy link
Member

For #6034

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).

@IanMatthewHuff
Copy link
Member Author

Per this issue here we need this in our preloads and renderers:
#6034

To test this I built VS Code with the special casing to still add the loader for ipynb removed. I tested without any changes first and simple widget rendering broke trying to load the widget (as expected). Making this change here fixes that.

I wanted to loop in @DonJayamanne with a draft here as I don't consider myself a JS module loading expert. Even with no changes and the loader removed I didn't see any issues with our renderers. Per the issue here require.js was needed to support amd module loading: microsoft/vscode#121245. However my change here (and the change in Jupyter) microsoft/vscode-notebook-renderers@f245b5a moved the renderers over to loading as a Javascript module. So I don't believe that we need a change for the renderers here, but I wanted to check my understanding there. My example test file with plotly output worked fine before and after removing the loader code from vscode core.

Note: I'm prepping a PR to remove the Jupyter special case for VS Code core using the initial change from Matt.

@IanMatthewHuff IanMatthewHuff marked this pull request as ready for review June 17, 2021 19:18
@IanMatthewHuff IanMatthewHuff requested a review from a team as a code owner June 17, 2021 19:18
@rchiodo
Copy link
Contributor

rchiodo commented Jun 22, 2021

@IanMatthewHuff is there a wait on this? I wanted to try it to see if allowed the 'notebook' renderer type for plotly to work again. See bug #6334

@IanMatthewHuff
Copy link
Member Author

@rchiodo Sorry just caught in the four day weekend. I need to check quick to see if the VS Code changes need to happen at the same time or not. Let me try it quick with current main. If it works there I'll push right away.
microsoft/vscode#126939

@IanMatthewHuff IanMatthewHuff merged commit 651c2a3 into main Jun 22, 2021
@IanMatthewHuff IanMatthewHuff deleted the dev/ianhu/requirePreload branch June 22, 2021 21:26
@IanMatthewHuff
Copy link
Member Author

@rchiodo I did a merge, not sure about it fixing the issue though. This just replaces vscode's loader with our loader which should be the same I think?

@rchiodo
Copy link
Contributor

rchiodo commented Jun 22, 2021

I'll try it out when the build comes out. The JS for plotly is failing because it's looking for the global 'require.undef'. I think adding require.js to our preloads should add that. It would explain why it works in the old webview notebook editor, it explicitly added require.js.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants