Skip to content
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

Revert "Don't depend on NIOFoundationCompat in NIOTransportServices on Linux (#209)" #210

Merged
merged 1 commit into from
Oct 14, 2024

Conversation

Lukasa
Copy link
Contributor

@Lukasa Lukasa commented Oct 14, 2024

This reverts commit 40ffcde in PR #209.

Unfortunately, Swift's behaviour around imports tends to be somewhat "leaky". In this case, the leak is that the dependency on the Package.swift allows downstream projects to import NIOFoundationCompat without needing to actually specify the package dependency. This has affected Hummingbird, which we noticed on our internal integration testing functionality:

hummingbird/Sources/Hummingbird/Codable/JSON/JSONCoding.swift:18:8: error: no such module 'NIOFoundationCompat'
import NIOFoundationCompat
       ^

cc @Joannis @adam-fowler for the Hummingbird report.

Unfortunately, we can't make this change until we're willing to use a semver major to achieve it. There may be some argument for doing that now, as the semver major will be very cheap to adopt across the ecosystem. But we'll need to do this in a considered way.

@Cyberbeni please feel free to reopen your PR targetting main, where we can discuss whether this is worth issuing a semver major for.

@Lukasa Lukasa added the semver/none No version bump required. label Oct 14, 2024
@adam-fowler
Copy link
Contributor

adam-fowler commented Oct 14, 2024

Ah that's frustrating as those imports are not needed. They were required by code that got removed. I've removed them from HB hummingbird-project/hummingbird#587

EDIT: I take that back they are needed

@adam-fowler
Copy link
Contributor

adam-fowler commented Oct 14, 2024

Would a better solution be to make NIOFoundationCompat use FoundationEssentials if it is available, or is that also going to have leak issues.

EDIT: Actually it can't as it uses JSONSerialization which is not in essentials.

@Lukasa
Copy link
Contributor Author

Lukasa commented Oct 14, 2024

Would a better solution be to make NIOFoundationCompat use FoundationEssentials if it is available, or is that also going to have leak issues.

Sadly, that also has leak issues. ☹️

We are likely to do a few semver majors in the nearish future to try to move from import Foundation to import FoundationEssentials where possible, but as you point out we can't do it for NIOFoundationCompat.

@Lukasa Lukasa merged commit fc398db into apple:main Oct 14, 2024
6 checks passed
@adam-fowler
Copy link
Contributor

We are likely to do a few semver majors in the nearish future to try to move from import Foundation to import FoundationEssentials where possible, but as you point out we can't do it for NIOFoundationCompat.

There could be an argument for deprecating the JSONSerialization support.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants