hawkerm / monaco-editor-uwp

A Windows Runtime Component wrapper around the web-based Monaco Editor.
MIT License
152 stars 36 forks source link

Updated to Monaco 0.20.0 #30

Closed hez2010 closed 4 years ago

hez2010 commented 4 years ago

Breaking changes:

hez2010 commented 4 years ago

@hansmbakker @Swiftpaws I've cherry-picked both your changes and updated monaco to 0.19.3 @hawkerm You can close #25, #26, #27 and #28 because this PR contains all the changes and fixes for them.

hez2010 commented 4 years ago

@hawkerm Could you please have a look at this PR? I've tested it and it works fine.

hez2010 commented 4 years ago

I've written a typedoc to C# converter which can be used for further model conversion. https://github.com/hez2010/TypedocConverter

You can first generate typedoc json for monaco, and then use TypedocConverter to generate C# bindings

hansmbakker commented 4 years ago

@hez2010 I haven’t tested your PR but it looks impressive what you did and achieved! According to what I read from it, you solved the outstanding blockers and took the PR further.

I can only say that I hope for you that @hawkerm / @michael-hawker will look at it, so that your contribution gets merged.

hawkerm commented 4 years ago

Wow, what a surprise @hez2010, thanks for moving us forward!

I'll pull this down this weekend, give it a whirl, and get it merged in.

hawkerm commented 4 years ago

@hez2010 we should also update the readme.md file too with the updates for building with VS 2019 and the bumped up target SDK version and such.

Looks like I also need to have the TypeScript compiler installed to build the project now? Do I just use the latest? Assuming I have to install this manually vs. referencing via NuGet?

hez2010 commented 4 years ago

@hez2010 we should also update the readme.md file too with the updates for building with VS 2019 and the bumped up target SDK version and such.

Looks like I also need to have the TypeScript compiler installed to build the project now? Do I just use the latest? Assuming I have to install this manually vs. referencing via NuGet?

Using the latest typescript compiler is okay. I tried TypeScriptCompile in csproj for automatical typescript compilation but it doesn't work for me. Therefore, I added a pre-build script to compile typescript files manually via invoking tsc. Currently we need to use npm install typescript -g to install typescript compiler first in order to build the project.

hawkerm commented 4 years ago

Thanks @hez2010. I'll take a look to see if I can just install the TypeScript installer from their website and if that'll work. Really would be nice to avoid an npm dependency.

hawkerm commented 4 years ago

I actually just added the NuGet package as described here and it appears to have worked, trying to clean my repo and test from fresh.

@hez2010 what's a good way for me to test this new code path?

hez2010 commented 4 years ago

I haven't written any unit test for it yet. Currently I only continued using the existing test project MonacoEditorTestApp for testing.

hawkerm commented 4 years ago

@hez2010 no worries, I didn't mean unit tests, just making sure that the compilation step was working. I saw that it built the appropriate files and used the test app to re-test things. Found something to fix for snippets, going to do a PR on top of your PR for your branch to merge back together with the easy build fix.

hawkerm commented 4 years ago

@hez2010 added my changes via https://github.com/hez2010/monaco-editor-uwp/pull/1

hawkerm commented 4 years ago

@hez2010 this is looking really good and exciting. I'm going to try and wrap my head around the actual changes and how it's working now, but functionality wise, we're looking pretty good.

Let me know on the other PR I setup if that's how I should add it now, or if there's a new way.

hez2010 commented 4 years ago

@hez2010 no worries, I didn't mean unit tests, just making sure that the compilation step was working. I saw that it built the appropriate files and used the test app to re-test things. Found something to fix for snippets, going to do a PR on top of your PR for your branch to merge back together with the easy build fix.

I think you can use Azure Devops Service for free CI and build the project automatically. I have script for building UWP projects.

hawkerm commented 4 years ago

@hansmbakker would you mind signing the CLA too as this was based off your original PR? Thanks! Just want to cover the code in case we end up moving this to the Windows Community Toolkit in the future.

hez2010 commented 4 years ago

I've made some improvements to Json models, I removed IJsonable interface and added an extension method ToJson() for all types.

hansmbakker commented 4 years ago

@hawkerm I signed the CLA. Hope this helps towards taking a next step with this control!

hawkerm commented 4 years ago

I've made some improvements to Json models, I removed IJsonable interface and added an extension method ToJson() for all types.

@hez2010 perfect, yeah that was left-over when I started out and thought I wouldn't need Json.NET... silly past me. Though hopefully eventually System.Text.Json will work with UWP, and we can switch over to that to try and minimize dependencies when .NET 5 hits.

I appreciate you continuing to tweak and fix things, and your patience for my slow reviewing.

I still need to carve out time to finish playing with the changes, testing them out, and going through the other half of the PR, but we're getting closer. I've been focused on a work project, so not sure how that's going to pan out; so if it's not merged in the next few days, I may have to wait until first thing in March. :( I do have it working on my machine, but haven't tried to build a package and test E2E yet.

@hansmbakker thank you so much, it's a big help! I'm just trying to cross all the 't's for the future since this is the largest contribution from anyone besides myself. I won't hold the PR on CLA for Swiftpaws as that was a small-line contribution; the bot is supposed to ignore those, but this is a mixed-author PR, so it probably got confused.

hawkerm commented 4 years ago

@hez2010 looks like they just shipped Monaco 0.20 (it's such a moving target). Luckily I don't see too many sweeping changes, so I'd imagine it'd be pretty straight-forward to flip over.

hez2010 commented 4 years ago

@hez2010 looks like they just shipped Monaco 0.20 (it's such a moving target). Luckily I don't see too many sweeping changes, so I'd imagine it'd be pretty straight-forward to flip over.

I did a quick look and updated to 0.20.0 via 5caff87. Seems that the monaco editor's CHANGELOG didn't cover all changes from 0.19.3.

Fortunately, with my TypedocConverter I can quickly generate a diff and apply changes with minimal efforts.

hez2010 commented 4 years ago

I think you can publish some prerelease builds to nuget.org before this PR fully tested and merged. It allows people to dogfood new features in advance and we can collect more feedbacks from issues.

hez2010 commented 4 years ago

Any updates on this PR? It has passed a month since last activity.

hawkerm commented 4 years ago

@hez2010 sorry for the delay, I was on vacation and actually unplugged a lot more than I expected (but that's always a good thing), but unfortunately caught a cold (not the current plague) and was out of commission last week. I'm jumping back on things, and I'll start getting back into this in earnest later this week.

avknaidu commented 4 years ago

@hawkerm will you be able to build a pre-release package once you confirm everything on this PR checks out?

hawkerm commented 4 years ago

@avknaidu yeah, I'm hoping to try and get this wrapped up soon. Once I do, I'll update the NuGet package.

hez2010 commented 4 years ago

@hawkerm I'm currently working on splitting Monaco type bindings into a single shared class library so that there will be no limitation of cannot expose unsealed public classes.

Updates: seems that there're some references to CodeEditor need to be resolved, postponed to further releases.

CLAassistant commented 4 years ago

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

:white_check_mark: hez2010
:white_check_mark: hansmbakker
:x: Swiftpaws
You have signed the CLA already but the status is still pending? Let us recheck it.