refactor(Observable): Make 'protected' of Observable<T>._isScalar

[tetsuharuohzeki]

This property should be accessed only from a derived class. There's no need to make this public.

[staltz]

LGTM

[benlesh]

I'm uncertain about this one... only because there are places where we're checking _isScalar on incoming observables (that just happen to be typed as any) at that point.

[benlesh]

...meaning, if we ever decided to use proper typing in those points (and I'm not sure why we would, since they're internal to the library and not public APIs), having this as protected would be a problem.

Is there a reason why we want this change?

[tetsuharuohzeki]

Is there a reason why we want this change?

Without this change, _isScalar field is visible as public field. I feel a developer will miss-touch this property, so we should hide _isScalar if we available to do it.
(Observable._subscribe has same problem, but we would need to define a new Symbol to hide it...)

[trxcllnt]

@saneyuki it's perfectly acceptable to override Observable.prototype._subscribe if you know what you're doing ;-)

[tetsuharuohzeki]

@trxcllnt

Yes, I don't think it's problem to override Observable.prototype._subscribe to create a new operator. I think it would be bad that Observable.prototype._subscribe could be accessed unconditionally from autocomplete or a developer who don't intend to create a new operator.

I feel it is reasonable idea to define a Symbol to access Observable.prototype._subscribe and export it to hook it.
(Umm, should we file a new issue to discuss about Observable.prototype._subscribe?)

[benlesh]

@saneyuki .... I think my concern is that we access it "publicly" internally and I don't want to have TypeScript complaining at us or need to have to cast something as any to get things to build. I already feel like I jump through a lot of hoops for TypeScript's sake.

[benlesh]

Sorry, @saneyuki, I think for now I'm going to close this PR. While it doesn't break anything, I'm not sure it fixes anything either. A third party may want to create a custom operator that optimizes for scalar observables which means checking _isScalar.

I really appreciate the thought and effort you've put into this. I'm just not sure it's the right move at this point.

... if we end up doing this later, you can totally say "I told you so" :)

[tetsuharuohzeki]

A third party may want to create a custom operator that optimizes for scalar observables which means checking _isScalar.

Then, they uses/extends ScalarObservable if they need to check _isScalar?

[benlesh]

PromiseObservable can also be _isScalar

[tetsuharuohzeki]

@Blesh

Ah, my words is too short. My intent of the previous reply is that:

A third party who want to create a custom operator for them can check whether scalar/promise observable by using instanceof checking instead of _isScalar property. Would they really look _isScalar property?

I think basically we should not make public of internal items until if we meet unavoidable to expose them. I'm worrying it would be harder to stop exposed internals in early stage.

[benlesh]

Since PromiseObservable can be optimized as "scalar" with _isScalar after it resolves the first time, and since PromiseObservable doesn't extend ScalarObservable, instanceof won't work.

However, maybe we can have PromiseObservable extend ScalarObservable... I don't know, I haven't put much thought into it.

[benlesh]

... probably not though.

[tetsuharuohzeki]

since PromiseObservable doesn't extend ScalarObservable, instanceof won't work.

This (specialized) pass would work as instanceof checking.

if (observable instanceof ScalarObservable || observable instanceof PromiseObservable) {
   ...
}

[tetsuharuohzeki]

...I meant it on my replying.

[benlesh]

I'll investigate it. I don't want to make this change right now. Other things would need to change. For example, you can't treat PromiseObservables the same as scalars until they've resolved once... so it would be more like:

if (observable instanceof ScalarObservable || (observable instanceof PromiseObservable && observable.isResolved)) {
  ...
}

I'm unsure if that's going to be more efficient than just checking isScalar.

Also, we'd have to publish ScalarObservable and PromiseObservable with the global package, otherwise people won't be able to test them unless they're using a module system so they can pull those types in.

[tetsuharuohzeki]

@Blesh

Okay. I accept your decision. Thank you for a long discussion.

[benlesh]

<3 @saneyuki