Add Kernel Extension for plotly formatter #54
Conversation
- Add dependencies for extension in own group because they are not stable yet - Add fix for GenericChart JS which breaks the embedded charts in VSCode - Add notebook for visually testing extension
|
Thank you for your work @WalternativE , i will check out the notebook tomorrow and play around a little before we chat about this. On first sight the only thing that i would maybe change is the namespace, 4 levels seems kinda deep |
Cool, looking forward to receiving your feedback 😊 Yeah, the namespace could be shorter. I just want to make sure, that it doesn't pollute other possible |
There was a problem hiding this comment.
Awesome! The notebook just works! I find it very interesting that you are actually building the library inside the notebook. I have only highlighted a few namespace changes that would simplify things (Plotly.NET.Interactive). Also, please take credit in the package metadata 😉 .
| @@ -60,7 +60,7 @@ module HTML = | |||
| newScript.AppendLine("<script type=\"text/javascript\">") |> ignore | |||
| newScript.AppendLine(@" | |||
| var renderPlotly = function() { | |||
| var fsharpPlotlyRequire = requirejs.config({context:'fsharp-plotly',paths:{plotly:'https://cdn.plot.ly/plotly-latest.min'}}); | |||
| var fsharpPlotlyRequire = requirejs.config({context:'fsharp-plotly',paths:{plotly:'https://cdn.plot.ly/plotly-latest.min'}}) || require; | |||
There was a problem hiding this comment.
I also saw this one in your blog post, could you give me some info why we need to do this? My js is 'basic' 😉
There was a problem hiding this comment.
My guess would be, that hoisting works a bit different in Electron from how it works in most browsers. When calling the immediately executed anonymous function to set up the plotly chart it tries to use the fsharpPlotlyRequire function and dies because it is undefined at the time. In other browser environments this doesn't happen. Why does this difference exist: beats me. You can try out how it behaves in VSCode (with the .NET interactive plugin) when you don't have this expression. It should work in your normal Jupyter environment, though. I can't talk about Nteract because I didn't test it there. Should also just happen for the recent versions of VSCode Insiders because they changed something there (my guess would be, that they switched to a newer Electron version, but I'm not sure).
There was a problem hiding this comment.
Well that's strange. I guess as long as this change does not affect the other environments we can take it.
| <?xml version="1.0" encoding="utf-8"?> | ||
| <Project Sdk="Microsoft.NET.Sdk"> | ||
| <PropertyGroup> | ||
| <TargetFramework>netcoreapp3.1</TargetFramework> |
There was a problem hiding this comment.
As this is a library, can we target netstandard?
There was a problem hiding this comment.
You are totally right! For some reason I thought, that the .NET Interactive nuget dependency was still using netcoreapp buth either they switched to netstandard2.1 or I just mixed it up (very likely 🙈). I'll change it to netstandard2.1.
- Shorten project name and related namespaces - Reflect changes in visual inspection notebook - Switch project to netstandard2.1
Yeah, it's super neat to have a way to directly test everything from the notebook. That's how they do it in the official repo. I hope, that one day it will get easier to use relative paths in .NET Interactive. I added all the changes you requested and retested the project. Hope I didn't forget anything. If there is something else missing, just let me know. |
because they are not stable yet
charts in VSCode
I'd be happy to chat about how it should be incorporated best into the standing project. This is a best effort PR right now so I'd imagine, that you want to have some things changed. I'd be happy to work with you on making this right for the project 😊