Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
JSON serialization #91
JSON serialization #91
Changes from all commits
f2a47dd
bfd3ccf
4f2f720
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Also add, we should re-order the parameters so that the object is first (as source) and the destination is second.
For these two APIs, PipeWriter and Stream should be second.
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.
What is the dependency graph going to look like for these?
This makes
System.Text.Json
to depend onSystem.IO.Pipelines
.System.Text.Json
is inbox,System.IO.Pipelines
is not inbox today.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.
That is a great question. We need to figure out the layering. If System.Text.Json depends on System.IO.Pipelines (which it would have to if we were to provide PipeReader/Writer overloads), it would force S.IO.Pipelines to be inbox. Is that even feasible?
cc @davidfowl, @pakrym, @steveharter
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 mean be part of Microsoft.NETCore.App? I think it should be in box and I was actually going to suggest it be in box outside of this conversation.
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.
Also on the System.Text.Json packaging we will likely need 3 build flavors:
Today we do 1 and 3.
The reason for 2 is the performance of using System.Reflection instead of IL.Emit causes the serializer to be much slower.
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.
What will .NET Framework use -- the source code only package? IL Emit is not supported by netstandard2.0.
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.
Yep. Our current plan is that only netcoreapp gets compiled assets for System.Text.Json
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.
OK so lets settle on this. System.IO.Pipelines will be in box in 3.0. We can file an issue to raise awareness in case there's pushback.
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.
Even if we put pipelines in box, I don't think the JSON component should depend on pipelines. Pipelines could depend on JSON though.
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.
No that’s wrong. Let’s discuss this outside of GitHub through. Pipelines should not become a dumping ground for all integrations with it.