This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
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.
Add new System.Net.Http.Json project/namespace #42889
Add new System.Net.Http.Json project/namespace #42889
Changes from 1 commit
fe82f34
43143ca
13ab13b
799aa67
af7770a
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.
We should omit the ref unless we have a good reason to have it.
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.
Our model until release/3.1 has been to always have the ref unless it causes issues (like hiding dependencies to RAR when targetting desktop) which this package isn't doing, so in my point of view it would be better to not special-case it and keep it consistent unless we think it would hurt. That said, I'm totally open for suggestions here so I can remove it too if you think we should.
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.
We previously did a pass to try to limit the number of places where we expose refs to desktop. System.Text.Json, for example, does not expose a reference assembly. All new packages we try not to include refs unless we must have them for some reason.
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.
should we also have a net461 configuration?
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.
We could add a net461 config, but that would still mean we will need the facades here, since we depend on SYstem.Text.Json which will pull them in. I was planning on adding net461 config here with my configuration changes wave comming next so that we have one PR doing it for every package that needs it. If you still think I should add it here now, I can do that too.
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.
Go ahead and do it now. I think the timing of the next wave will land before we stabalize this, so I'd prefer to have this done WRT coding and just need to pick up package dependency updates when we ship those.
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.
(assuming this is a small task, if anything gets complicated we can postpone this until after the first preview)