fsprojects / FSharp.Data

F# Data: Library for Data Access
https://fsprojects.github.io/FSharp.Data
Other
814 stars 288 forks source link

Move non-http code from FSharp.Data.Http into a new FSharp.Data.Runtime.Utilities project + Rearrange project files #1475

Closed mlaily closed 1 year ago

mlaily commented 1 year ago

closes https://github.com/fsprojects/FSharp.Data/issues/1468

For the record, I used the name suggested in the linked issue above for the new common project, but I feel like something like FSharp.Data.Runtime.Common might have been more appropriate.

This is still an improvement over the current situation with everything in FSharp.Data.Http.

I also took the opportunity to rearrange source files into their respective project folders.
Files layout in the src folder should now mostly match the solution organisation.

This also has the nice side effect of making folders reappear in Visual Studio. (for some reason, files included in projects but living outside the project's folder appear as a flat list of linked files in this IDE)

The complete diff is ugly, but reviewing individual commits should be more comfortable.

mlaily commented 1 year ago

The Windows build seems a bit flaky... It's currently failing because it couldn't properly download an rss feed in the docs fsx...

Is there a way to simply retry it?

mlaily commented 1 year ago

@cartermp hey, thanks for taking a look! 🙏

I have a commit ready to shut-up the worldbank api-related test failures.

You have just a word to say and I can add it to the branch of this PR!

(if you want to take a look first: https://github.com/fsprojects/FSharp.Data/commit/f1115e3da741e6ecc2dfb9c5d47eb9d6cc361bf7)


EDIT: ah, it seems there is another network-related error with the html provider docs -_-
I'll take a look...

EDIT2: The html provider error is caused by the following difference when requesting the https://www.nuget.org/packages/FSharp.Data page 😩

image

mlaily commented 1 year ago

Well, looks like the last build failure was a TLS version issue...

For the record:

docs\library\XmlProvider.fsx: error(410,11)-(410,51) The type provider 'ProviderImplementation.XmlProvider' reported an error: Cannot read sample XML from 'http://tomasp.net/rss.xml': System.Net.WebException: The SSL connection could not be established, see inner exception.
 ---> System.Net.Http.HttpRequestException: The SSL connection could not be established, see inner exception.
 ---> System.Security.Authentication.AuthenticationException: Authentication failed, see inner exception.
 ---> System.ComponentModel.Win32Exception (0x80090304): The Local Security Authority cannot be contacted
   --- End of inner exception stack trace ---
   at System.Net.Security.SslStream.ForceAuthenticationAsync[TIOAdapter](TIOAdapter adapter, Boolean receiveFirst, Byte[] reAuthenticationData, Boolean isApm)
   at System.Net.Http.ConnectHelper.EstablishSslConnectionAsync(SslClientAuthenticationOptions sslOptions, HttpRequestMessage request, Boolean async, Stream stream, CancellationToken cancellationToken)
   --- End of inner exception stack trace ---

I'm not sure why it would not be enabled on the github windows runner, but adding TLS 1.2 to ServicePointManager.SecurityProtocol in Http.fs fixed the build.
Since it doesn't seem like a very good idea to do that in library code, I enabled it in the design time provider code instead.

All in all, I added four commits to the branch to fix the build.
Sorry about the noise and the notifications you probably received! ^^"

Adding aria-label to the list of html attributes used by the HtmlProvider to infer names is technically a breaking change, but it's also a nice improvement for pages that support it like nuget.org!

I hope this is ok!

cartermp commented 1 year ago

Thanks @mlaily. There sure is a lot of ... "legacy behavior"... in here 🙂