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
Avoid FoundationNetworking and libcurl on non-Darwin platforms #681
base: main
Are you sure you want to change the base?
Avoid FoundationNetworking and libcurl on non-Darwin platforms #681
Changes from 4 commits
66353a1
0db65fa
c1b5529
992d221
58a5d0e
f042a68
71fe790
8c4e6d0
fb38bef
f2b5908
ea5bccb
98561ac
30500a0
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.
This is a source breaking change. Is there anywhere to maintain both declarations to ease this transition?
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 can guard the old function that uses
URLRequest
andURLResponse
on#if canImport(Darwin)
if that's ok? Otherwise its presence on non-Darwin platforms adds a dependency onFoundationNetworking
with curl and the rest of the system networking/crypto libraries, which is what this PR was meant to remove in the first place.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'm not sure. I'm concerned that someone could have an existing setup on Linux with curl and the rest of the system networking libraries and that their setup would break when they update to a new DocC version.
We try to provide a transition period for breaking changes. If that's not possible then so be it (although we would try to inform people about it ahead of time in the Forums). But it there's a way to stage this, then I think that it would be less disruptive that way.
Would it be possible to use a compile time define to exclude FoundationNetworking? That way we could have a 3 stage transition (first opt-in to exclude, then exclude by default, and finally remove it completely)
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.
Can you explain why this isn't a change in behavior? What's needless about this?
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.
Only
path
component ofurl
thatdata(for:)
took as argument was used. The rest of the components are ignored by DocC server implementations.Additionally,
HTTPRequest
unlikeURLRequest
doesn't store full URLs, only paths. The authority and scheme components can be assembled fromscheme
andauthority
properties onHTTPRequest
, but those aren't used anywhere in the DocC codebase, other than a test in this diff below where we have to construct a newURLRequest
to pass it toWKWebView
on Darwin platforms.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.
But if
request.path
is"/something"
then this used to pass"/something/index.html"
as an argument but now it only passes "index.html". Isn't that returning data for the wrong path?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.
If
request.path
is"/something"
then that is handled by theif
branch above, no thiselse
branch. The latter branch didn't userequest.path
previously, that logic is unchanged, as verified by the test suite.