search

groue / GRDBCombine

GRDB ❤️ Combine
MIT License
223 stars 16 forks source link

Use CXShim instead of hardcoded Combine (support Linux / macOS 10.10 / iOS 8.0) #26

Closed ddddxxx closed 4 years ago

ddddxxx commented 4 years ago

CXShim is a virtual Combine API that allows end user (not library author) to choose concrete implementation. see Combine Compatible Package.

I'm exploring the real world use case of CXShim. SwiftWebUI/SwiftWebUI#49 is my previous attempt.

Advantage

I'd like to work on a pr with your permission.

groue commented 4 years ago

Hello @ddddxxx,

Thanks for your interest in GRDBCombine.

If I would accept a pull request, the responsibility for supporting GRDB and GRDBCombine users on Linux / CombineX would be belong to the GRDB and GRDBCombine stewards. I'm talking about very precise tasks such as writing documentation, automating tests, solving user problems, answering and closing GitHub issues, etc.

This is more work. Over the years.

So, no, thank you, I'm not interested in a pull request yet. Nothing prevents you from forking GRDBCombine. When you are ready to become a long-term GRDB and GRDBCombine steward, and are committed in supporting GRDB and GRDBCombine users, please chime in.

ddddxxx commented 4 years ago

I believe the responsibility is belong to CombineX community. CXShim is a complete solution for porting every Combine-related project to every platform. If a library works fine on Catalina with Combine, it must work on linux with CXShim. Or CXShim is doing wrong and needs to be fixed. In other words, I have already took the responsibility.

Other Combine-unrelated problem, like using NSMapTable on linux, is out of my scope however. If you count this as "long-term steward", I'd like to chime in.

My initial pr will update the CI to run existing test suit on linux.

groue commented 4 years ago

If a library works fine on Catalina with Combine, it must work on linux with CXShim.

You forgot to mention this, quoted from https://github.com/cx-org/CombineX:

Though CombineX have implemented all the Combine interface, the project is still in early development and not ready for production.

You also forgot to mention that there may exist other Combine reimplementations, and I don't know why CombineX should be favored over other ones.

I do worry we did not quite understand each other. Please read again my previous answer, especially the sentence about "very precise tasks".

So let me rewrite my conclusion: fork GRDB and GRDBCombine. Maintain your forks. Keep them up to date. Answer issues. Solve problems. When you have done that for a while, reliably, I may consider merging our common efforts.

groue commented 4 years ago

Finally, something is completely reversed in your request. What interest is there using GRDBCombine with CombineX when GRDB is not supported on Linux? This has no sense.

ddddxxx commented 4 years ago

GRDB support macOS 10.10 / iOS 8.0 / tvOS 9.0 / watchOS 2.0 and GRDBCombine don't.

ddddxxx commented 4 years ago

You also forgot to mention that there may exist other Combine reimplementations, and I don't know why CombineX should be favored over other ones.

I'm suggesting CXShim, not CombineX. CXShim support all existing Combine reimplementations AFAIK, and will support any future Combine reimplementations.

ddddxxx commented 4 years ago

If you use any other concrete Combine reimplementations, even using CombineX, you need compilation conditions everywhere. With CXShim, you don't.

If you known how can I use open-source Combine in lower version macOS without compilation conditions and shim package, please do let me know. I'd like to improve.

groue commented 4 years ago

GRDB support macOS 10.10 / iOS 8.0 / tvOS 9.0 / watchOS 2.0 and GRDBCombine don't.

So this is not only about Linux. This is interesting, and useful. No GRDBCombine user has asked for this yet, though.

I'm suggesting CXShim, not CombineX. CXShim support all existing Combine reimplementations AFAIK, and will support any future Combine reimplementations.

This is Interesting as well.

Please fork and maintain your forks, all right?

ddddxxx commented 4 years ago

Combine on linux and lower version OS of Apple platform is a real problem. I want to solve it.

I'm still exploring possible solution. CXShim is the best I can do. I understand you value stability very much (the issue level of GRDB is incredibly low). And you don't like to try experimental technic on a huge project like GRDB.

How about I fork this project, add CXShim support. You add a link to my fork, state that it's an unofficial support for back deploy?

No GRDBCombine user has asked for this yet, though.

A lot of times, people don't know what they want until you show it to them. -- Steve Jobs

OK I'm joking. But if you support it, people will try. Users and feedbacks is the most important for CXShim for now.

groue commented 4 years ago

OK now we understand each other 👍. Now please let me breathe for a while and think about it more. Thanks.

ddddxxx commented 4 years ago

Take it easy. I'm not meant to be pushy.

If you'd like to see a real world example of CXShim, take a look at my project.

ddddxxx commented 4 years ago

Forked https://github.com/cx-community/GRDBCombine, also https://github.com/cx-community/CombineExpectations.

The test suit passes against both Combine and CombineX.

The demo app works fine with Combine, yet CombineX is not applicable due to SwiftUI integration.

OpenCombine is currently not available. Because it doesn't support Foundation extensions, uniform namespace and compatible package.

groue commented 4 years ago

That's great, @ddddxxx. Users who will look for Combine alternative will be able to find your forks.

Right now, YAGNI advises me not to import such a diff. Happy maintenance of CXShim, CombineX, and all the projects you fork!

ddddxxx commented 4 years ago

Hi @groue , I'd like to share recent update regarding my proposal.

I wrote an article about CXShim: https://forums.swift.org/t/combine-backward-deployment-and-linux/37855. Some points may not apply to GRDBCombine, like you already have GRDB and don't expect benefit from Combine. But I think it's overall fair argument (I didn't get enough feedback, however).

Now we already have GRDBCombineX and GRDBOpenCombine. And some project start to use third party combine even they are targeting on macOS 10.15. I think now the ecosystem issue is a thing.

It have been 7 month since CXShim released. And I've released my LyricsX app 3 times with CXShim integrated for 4 month. Everything works just fine.

I understand it's hard for you to adopt such fundamental change with potential stability issue. And it's not fair for GRDBCombine users to take unnecessary risk. Instead, I propose we adopt CXShim for CombineExpectations package first, for following reasons:

  1. CombineExpectations is only used by test suit. The existing users are literally unaffected.
  2. It's a perfect example that shows adopting CXShim is purely additive and doesn't break anything (I suspect a patch version bump is sufficient). GRDBCombine won't need to change a single line of code, package description, project configuration or CI script.
  3. CombineExpectations will get more audience from CombineX and OpenCombine ecosystem.
  4. CombineExpectations can also benefit from CXTest. (Or you can just contribute to CXTest, but that's out of scope of my proposal)

Once you feel confident about the change of CombineExpectations, we can talk about migration of GRDBCombine. What do you think?

groue commented 4 years ago

Hello @ddddxxx,

I'm very busy shipping GRDB 5 currently, and this is my primary focus. GRDBCombine will disappear as a standalone library, and will be merged in GRDB 5. CombineExpectations will continue to exist, but will be copied into GRDB so that the main library remains dependency-free and can easily support the various installation methods (SPM, CocoaPods, manual installation, custom SQLite build, SQLCipher)

In order to embed GRDBCombine inside GRDB, which has iOS 10.0+ / macOS 10.10+ / tvOS 9.0+ / watchOS 2.0+ requirements (far behind what's necessary for Combine), I have to use a mix of #if canImport(...), @available(...), if #available(...), as you can imagine.

This means that I don't have any second to spend at thinking about CXShim or any other Combine replacement that exists out there, I'm sorry. I don't have the time to read. I don't have the time to evaluate alternatives. I don't have the time to dance the #if and @available jigg. I don't have the time to package and document all this stuff. I don't have the time to tests all this stuff. I don't have the time to maintain all this stuff in the long run. I don't have the time to provide support for all this stuff when people have questions.

So, I'm sorry @ddddxxx, but I'm really not interested. Some other people may be, who knows? They'll spend their time as they wish, as you and I both do.

As I said above:

Happy maintenance of CXShim, CombineX, and all the projects you fork!

ddddxxx commented 4 years ago

I see. Thanks for your reply.

I don't have the time to dance the #if and @available jigg.

I'd say that's exactly what CXShim tries to solve (and solved).

groue commented 4 years ago

I'd say that's exactly what CXShim tries to solve (and solved).

Sure, but only in the case some Combine implementation is available. Now think about all the people that dont care about Combine and won't add any dependency on Combine or any Combine replacement.