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

1.x: Deprecate Observable.create() #4253

Closed
wants to merge 1 commit into
base: 1.x
from

Conversation

Projects
None yet
5 participants
@artem-zinnatullin
Copy link
Contributor

artem-zinnatullin commented Jul 28, 2016

Finally we have Observable.fromAsync() and it's time to prevent users from using Observable.create().

@akarnokd

This comment has been minimized.

Copy link
Member

akarnokd commented Jul 28, 2016

Don't. Makes all legitimate uses now show up as warnings, including all RxJava!

@artem-zinnatullin

This comment has been minimized.

Copy link
Contributor

artem-zinnatullin commented Jul 28, 2016

But we have to. It's too dangerous and people keep using it in tutorials for beginners!
@JakeWharton is trying to teach them all not to do so (👍) but @Deprecate is a much more efficient and better way to do it.

Makes all legitimate uses now show up as warnings

Pretty sure most of them don't support backpressure -> legitimate == false!

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 28, 2016

Current coverage is 84.24% (diff: 100%)

Merging #4253 into 1.x will decrease coverage by 0.09%

@@                1.x      #4253   diff @@
==========================================
  Files           266        266          
  Lines         17446      17446          
  Methods           0          0          
  Messages          0          0          
  Branches       2657       2657          
==========================================
- Hits          14715      14698    -17   
- Misses         1870       1883    +13   
- Partials        861        865     +4   

Powered by Codecov. Last update 41959f7...ea7300d

@vanniktech

This comment has been minimized.

Copy link
Contributor

vanniktech commented Jul 28, 2016

If so should not SyncOnSubscribe & AsyncOnSubscribe be mentioned too?

@akarnokd

This comment has been minimized.

Copy link
Member

akarnokd commented Jul 28, 2016

This I why I use Eclipse: it's immediately obvious what the cascading effects of changes are:

image

The javadoc contains the warning about using the method:

This method requires advanced knowledge about building operators and data sources; please consider other standard methods first;

@artem-zinnatullin

This comment has been minimized.

Copy link
Contributor

artem-zinnatullin commented Jul 28, 2016

We can suppress them in RxJava!

Sometimes to save someone you love you need to sacrifice something 🚢

@akarnokd

This comment has been minimized.

Copy link
Member

akarnokd commented Jul 28, 2016

You may not care about the impact but I have to. This change is too radical; a small deprecation now amplified to 400+ warnings and would need to suppress several hundred places - all legitimate (operators) or acceptable uses (unit tests for corner cases).

👎

@artem-zinnatullin

This comment has been minimized.

Copy link
Contributor

artem-zinnatullin commented Jul 28, 2016

I better suppress 400+ warnings than let one user to misuse Observable.create() and get MissingBackpressureException or allow emission without subscriber.isUnsubscribed() check.

I understand that you care about library 👍, but I care about users in this PR!

Only in open source projects with shitty GitHub search I found about 6k+ usages of Observable.create(), are you sure they all correctly handle backpressure, errors and unsubscription?

Recently we had to handle backpressure with Observable.create() in our library StorIO, same was done in SQLBrite and Retrofit. And this is because maintainers of those libraries were aware of MBE and how to handle it, others may not have required level of understanding to correctly use Observable.create(). (I try to avoid it as much as possible).

@akarnokd

This comment has been minimized.

Copy link
Member

akarnokd commented Jul 28, 2016

Still you don't just deprecate something and leave the fallout to other maintainer(s).

Instead of just deprecating create this is what I'd do:

  • refactor-rename create to build in the entire project -> keeps all our use places intact, no suppressing needed
  • copy the build method back to create
  • mark create as deprecated, add pointers to the alternative methods in the Javadoc
@artem-zinnatullin

This comment has been minimized.

Copy link
Contributor

artem-zinnatullin commented Jul 28, 2016

That sounds better to me, I was thinking about package private method inside Observable, like createInternal() + rx.Internal.createObservable(), if it would be required, to completely hide it from users (and deprecate Observable.create()).

Does it sound good to you @akarnokd?

If so should not SyncOnSubscribe & AsyncOnSubscribe be mentioned too?

Up to @akarnokd.

@akarnokd

This comment has been minimized.

Copy link
Member

akarnokd commented Jul 28, 2016

Just making it package-private does not work. We have accesses from other packages that require the create feature. It means we'd have to dump them into the main rx, all their tests. With Internal, now you have a publicly accessible class and just switched Observable.create with Internal.createObservable. Java 6's visibility rules are simply not powerful enough to hide create.

@artem-zinnatullin

This comment has been minimized.

Copy link
Contributor

artem-zinnatullin commented Jul 31, 2016

So, after discussing that in Twitter looks like our steps could be:

  1. Teach users to use fromAsync() in their apps but not in libraries yet because it's @Experimental.
  2. Collect feedback.
  3. Stabilise fromAsync() and promote it to @Beta or stable.
  4. Deprecate create().

Regarding hiding it, I think internalCreate() or unsafeCreate() will do the work.

If that sounds good to you, I'll close this pr and start working on things from list.

@akarnokd

This comment has been minimized.

Copy link
Member

akarnokd commented Jul 31, 2016

That sounds good.

@davidmoten

This comment has been minimized.

Copy link
Contributor

davidmoten commented Jul 31, 2016

Regarding hiding it, I think internalCreate() or unsafeCreate() will do the work.

Deprecating create seems like a good idea to get users to consider what they're doing but the create functionality still needs to be part of the public API. unsafeCreate may be a useful name to make people think twice about it.

Re fromAsync teaching, one outstanding issue with it was request batching. @akarnokd submitted an operator that we can use for that (rebatchRequests) and I suspect it should be covered as part of the documentation of fromAsync usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment