chore: use @internal and _ for internals; remove protected modifiers

[cartant]

Description:

Update: this PR now does what's listed in this comment.

---

This PR removes the @deprecated tags from public/protected internals in favour of using @internal tags along with a underscore prefix - the latter was already used extensively throughout the codebase, so this just makes things more consistent, IMO, and means that people looking at runtime objects in the console or debugger will see properties and methods that are adorned with underscores that scream "THESE ARE INTERNAL".

Personally, I find that using the @deprecated tag for internals just makes the RxJS source harder to work with - as many things are shown struck out in VS Code. IMO, using @internal - there is an ESLint rule that'll use that tag - and an underscore is more than enough to indicate to developers that something should not be relied upon or messed with.

I've not removed @deprecated from the lift API, that used to be public, albeit undocumented. And I've not removed it from the exported Operator and Scheduler types.

This PR also removes the protected modifiers that were re-introduced in the v7 refactor/rewrite. This avoids the problem that was addressed in #3544 - which removed the protected modifiers. (TS Playground repro of the problem addressed in that issue here.) I've not removed them from types that are not exported - like the scheduler actions.

Whether or not the removal of protected is the what ought to be done, IDK. I've removed it in case it was added without what was done in #3544 being considered. However, IMO, I think removing it - inline with what was done in #3544 - will be the least problematic option. ATM, in v7, the lift API is protected and I think if it's left like that, problems are going to be effected, as Observable is a type that's likely to be exported by user-land packages.

Related issue (if exists): #5967 (kinda)

[benlesh]

Ugghhhhh.... This sucks. I see what you're doing, I agree with what you're doing. And I hate it. All at the same time.

Shouldn't the @internal annotation prevent these from being emitted, though? What does it look like in the IDE? I rather liked that people got an annoying "slash" through things they shouldn't be using. I'd even more rather it be a type error, TBH (hence protected).

Also, #3544 was a long time ago. Are we sure this is still an issue?

[cartant]

... prevent these from being emitted, though?

There is a TS thing that relates to not emitting internals, but it's not supposed to be used and using causes problems.

Are we sure this is still an issue?

Yeah, the TS Playground shows that it will be.

A better solution - as you've noted before - would be to not export classes.

[benlesh]

Along with my other comment about observers, specifically, we really need to think about this.

The problem with what I see here is we're renaming things (rather than just affecting their TypeScript visibility), which is a run-time breaking change.

For any JavaScript (non-TS) user, the API that they thought was theirs to use, isStopped for example, is now suddenly _isStopped. Which, to some degree is fine but we're going to need to document it.

Some part of me really wants to make sure that #3544 is even an issue in 2021, given that it was dealt with back in 2018. On the surface, this PR looks like we're moving the wrong direction, if we don't have that context.

[benlesh]

Yeah, the TS Playground shows that it will be.

I'm stupid, where is the playground? I don't see it.

[benlesh]

I feel like what I've learned from this is that protected is a joke when it comes to writing libraries, and shouldn't be used.

[cartant]

(TS Playground repro of the problem addressed in that issue here.)

protected is the problem. Types in different package installations will be deemed different, so they have to be compatible.

[benlesh]

As a more moderate maneuver, could we only rename the properties we had deprecated in 6.x, and deprecate the properties we hadn't renamed yet for v8? Then we can remove/rename those in v8 without as many issues as doing it right now.

[cartant]

As a more moderate maneuver, could we only rename the properties we had deprecated in 6.x, and deprecate the properties we hadn't renamed yet for v8?

Sure. This PR rolls up several ideas. We can pick and choose.

[cartant]

I feel like what I've learned from this is that protected is a joke when it comes to writing libraries, and shouldn't be used.

Just go all the way: OO is a joke. 😅

[benlesh]

Another alternative: What if we keep the properties protected, but use an interface of the public properties anywhere we're accepting these things? It seems like that's the better thing to do? I'm probably wrong, but I want to know why I'm wrong.

[cartant]

@benlesh

As a more moderate maneuver, could we only rename the properties we had deprecated in 6.x, and deprecate the properties we hadn't renamed yet for v8?

I found the comment that I'd remembered and, AFAICT, we should do this:

• Tag previously deprecated, internal APIs as @internal. And ensure they're prefixed with underscores (already done in this PR) as that is the convention that api-extractor insists upon.
• Use --stripInternal to remove them from our .d.ts files. (Unfortunately, we cannot use api-extractor because of its single import location limitation.)
• Revert the removal of protected from APIs that are tagged with @internal, but any non-@internal APIs that were protected prior to this PR will have to remain public.
• Revert the renaming and @internal tagging of internal APIs that were not tagged as @deprecated in v6 - tag them as @deprecated instead - and wait till v8 before tagging them as @internal.

I'll look at making these changes.

[cartant]

I tagged _subscribe as internal 'cause it was tagged in many other places and this was done as part of a search/replace thing. I've ignored the other protected members in this class 'cause the entire class is deprecated.

[cartant]

I wanted to double check that the protected thing has been addressed, so I had a quick peek at the api_guardian-generated files and, AFAICT, the only remaining uses of protected are in ConnectableObservable, Subscriber (the ctor of which is deprecated), and VirtualAction. So we should have no protected-related, multi-package, type interop issues now that @internal has been used.