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

Adding plain MVC example (non-reactive) #356

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

monishsyed
Copy link

@monishsyed monishsyed commented Aug 8, 2020

Hello @rudro @neakor and others,

Firstly, thanks for creating this wonderful DI library that has certainly benefited many many devs and many many companies like ours. We recently started using Needle and we already feel this is so much better than any other DI systems out there primarily due to compile-time safety and for its hierarchical structure. Thank You!

While onboarding, many devs (#251) in the community have pointed out that for ease of understanding it would be great to have a non-rx version of the sample included. And therefore, I took a stab at copying the existing implementation and creating a non-rx version of the MVC sample.

I did minimal changes to the design itself since I think it would not make sense to redesign the app given its just a sample and wouldn't be used for anything other than learning the usage of Needle. I just migrated the app to use old-style broadcaster/listener. Hope it all makes sense. Looking forward to hear back :)

…rchitecture (non-reactive)

- Added old style broadcasters and listeners to broadcast messages to "interested" listeners
- Renamed old "MVC" folder to "MVC-rx"
- Updated README.md to document this change
- Updated RootViewControllerTests (apologies that I didn't add any tests given that this is just a sample and old sample didn't have enough tests either apart from RootViewControllerTests)
- Removed RxSwift and RxCocoa dependency completely (from cartfile as well)
@CLAassistant
Copy link

CLAassistant commented Aug 8, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

@rudro rudro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I have some very minor comments about the code (e.g you're copying the scorestream and some others but they are no longer needed in this version).

I think the bigger question is the value of this sample. I don't have strong feelings about this so we can merge if you feel strongly that it adds value.

Here are my toughts:

  • There was a time RxSwift was a niche thing, but now in the world of SwiftUI and Combine, I'm not sure that there will be as many developers not familiar with reactive programming. Rx's API is quite similar to Combine, and once people learn combine, they should at least get the notion of subscribing to a stream.
  • In this diff, you are essentially creating a super lightweight version of Rx with no operators of any kind. It is nice in that it's a tiny amount of code that it's easy to read and parse. On the other hand, we still have the concept of an update (onNext) and adding a listener (subscribing in the Rx context). So I feel the concepts are still there, but yes, the implementation is compact and easy to understand.

@monishsyed
Copy link
Author

Hi @rudro ,

Thanks for taking out time for review and sharing your concerns. Very much appreciated!

While I partially agree to your concerns, there is additional context to it. Let me share some of those here.

  1. I have been lately working on some internal and external talks on Needle and a significant number of people have shared feedback that understanding a tech becomes harder when samples are built using a specific paradigm, design, library etc (even when they are comfortable with those, I think it just takes away the focus from Needle to learning or understanding something else).
  2. Similar to us, there are many companies who still don’t use Rx (and some of them go as far as hating it 😅, I promise we are not one of them 😀 ) - and honestly there are genuine reasons for it (some of it me or you may not agree with). This along with Add Commander package for command-line parsing #1 could greatly impact adoption of the solid work that the team at Uber has done with Needle.
  3. Its always better to keep as few dependencies as possible for other reasons such as maintenance cost etc. This is even more true for OSS as the use cases (tech stack) may vary greatly between companies and developers. Rx is just a dependency on the Sample app (not on the library itself) so its not an issue here though.

The broadcaster/listeners I added may look like a super lightweight version of Rx in terms of behavior, but IMHO Rx is completely different paradigm of asynchronous programming which a lot of devs may not be comfortable with YET.

If you think this isn't adding much value, feel free to close the PR.

@shali3
Copy link
Contributor

shali3 commented Jan 17, 2021

I tried to reduce friction by removing carthage:
#383


// MARK: - High Scores

private func buildScoerButton() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private func buildScoerButton() {
private func buildScoreButton() {

view.backgroundColor = UIColor.purple

buildCollectionView()
buildScoerButton()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
buildScoerButton()
buildScoreButton()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants