-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[AOT compatible] Clean up some AOT build issue in FilePreviewCommon and MarkdownPreviewHandler #36207
base: main
Are you sure you want to change the base?
Conversation
…ion. Fix json serilizer aot issue.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
I encountered some build issue when I was trying to build installer locally. That's the reason why I need to wait for pipeline complete... |
Hi @moooyo , I usually build the installer to give it a test through these incantations in the "Developer Command Prompt for VS 2022`:
Hope this helps. |
Thanks!!! It works. |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
src/modules/previewpane/MarkdownPreviewHandler/MarkdownPreviewHandlerControl.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…HandlerControl.cs Co-authored-by: Jeremy Sinclair <[email protected]>
Co-authored-by: Jeremy Sinclair <[email protected]>
Accept all suggestions. Thanks @snickler ! |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Hi, we've added the "Don't Merge" tag to the PR, since we're trying to keep the repo stable for the release and possible hotfix if necessary. Please don't merge the PR while the tag is still in here. This allows people to review the PR and approve with less fear that it'll get merged 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may make decision to change it now, or in another PR.
|
||
[JsonSourceGenerationOptions(WriteIndented = true)] | ||
[JsonSerializable(typeof(JsonDocument))] | ||
internal sealed partial class FilePreviewJsonSerializerContext : JsonSerializerContext |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember I see this file twice on different location.
Looks like we need somewhere more centralized but not per project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yeelam-gordon each class implementing the JsonSerializerContext
class has to be in a spot where each of the class types are available to it. There are many types that are in the Settings project, but some types that are only in the individual projects. This one though, would actually be a good one to be moved to a more centralized location.
Summary of the Pull Request
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed