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

Consider a different API #30

Closed
JulianKingman opened this issue Jun 26, 2019 · 7 comments
Closed

Consider a different API #30

JulianKingman opened this issue Jun 26, 2019 · 7 comments

Comments

@JulianKingman
Copy link

The API for pubnub react is at odds with standard react patterns. For that reason I find myself using the JS SDK and ignoring this package, and wishing it were something different. This is what I wish this package did:

  • Initialize PubNub in a more standard way, so it can be moved to a startup script. The way it's initialized in the constructor and ties in with the class (in mysterious ways) is strange, and doesn't work with pure functions. I'm also confused about how to use it in separate components. Do I instantiate it once per component?
  • Use react context, and provide a PubNubProvider Provider component (accepts initiated pubnub instance) and withPubNub consumer
  • withPubNub provides standard PubNub API functions, like subscribe, publish, etc...
  • When subscribed, withPubNub initializes a listener behind the scenes and passes message data in as props

I don't really expect that this will be implemented, but thought it might be useful feedback, and would make this much more intuitive to react and react native developers.

@davidnub
Copy link
Contributor

@JulianKingman Thank you so much for your comments! We've recently started re-analyzing some of our SDKs and the React one is the first one that we've noticed wherein your comments are exactly the same as our observations.

In other words, we completely agree.

Rest assured we're in the process of planning out a newer refactor to this SDK such that it follows the standard React patterns better and feels more natural to use. We really appreciate your feedback and will take them into consideration as we plan further.

@JulianKingman
Copy link
Author

@davidnub if you want some inspiration, I'm working on a Provider/Context API internally for my app using the JS package here: https://github.com/DesignmanIO/trainerengine-mobile/tree/master/Components/PubNubContext

It's a work in progress, but may give you some ideas. Feel free to copy/paste.

@davidnub
Copy link
Contributor

davidnub commented Jul 8, 2019

Thanks @JulianKingman . I'll keep that in mind when we start reviewing options for change.

@ottoo
Copy link

ottoo commented Aug 2, 2019

I came here to say the exact same thing as @JulianKingman. The current API feels a bit off for the reasons already mentioned above. I have also taken a similar React Context based approach in my application that works pretty well with the JS SDK.

@davidnub
Copy link
Contributor

@JulianKingman and @ottoo Just wanted to inform both of you of our recent beta release of React 2.0 https://github.com/pubnub/react/tree/v2.0

I hope this is a good start to what you guys might be expecting from such an SDK.

Looking forward to hearing additional feedback and suggestions.

@JulianKingman
Copy link
Author

@davidnub yay, it looks really good! Can't wait to try it.

@davidnub
Copy link
Contributor

Thanks, there are still some extra things to add to it (ie. More Hooks etc) but hoping to get things flushed out as feedback comes in as well. I'm going to close this Issue since it has been considered and we're working towards it. Feel free to open more or reach out as you have more feedback and suggestions.

Thanks again for the comments!

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

No branches or pull requests

3 participants