-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
feat: remove uses of rxjs patch operators #5314
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few comments
src/lib/core/rxjs/rx-chain.ts
Outdated
* @docs-private | ||
*/ | ||
export class RxChain<T> { | ||
constructor(private _context: Observable<T>) { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also make the constructor private
so people have to use the static from
and get a strict chain.
(just looked this up a moment ago)
src/lib/core/rxjs/rx-chain.ts
Outdated
|
||
/** | ||
* Utility class used to chain RxJS operators. | ||
* @docs-private |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe expand the comment with something like
/**
* Utility class used to chain RxJS operators.
*
* This class is the concrete implementation, but the type used by the user when chaining
* is StrictRxChain. The strict chain enforces types on the operators to the same level as
* the prototype-added equivalents.
*
* @docs-private
*/
src/lib/core/rxjs/rx-operators.ts
Outdated
|
||
/** | ||
* Represents a strongly-typed chain of RxJS operators. | ||
* @docs-private |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expand to something like
/**
* Represents a strongly-typed chain of RxJS operators.
*
* We achieve strict type enforcement on the chained operators by creating types that
* *unambiguously* match specific rxjs operators. These unambiguous types are created by
* intersecting a "brand" to the `typeof` the existing operator. The brand (a class with a private
* member) effectively forces nominal typing for the operators. This allows typescript to understand
* that, for example, `filter` is *`filter`* and not, say, a map of T => boolean.
*
* The downside to this approach is that operators must be imported in their type-coerced form
* rather than from the normal rxjs location.
*
* @docs-private
*/
Addressed the feedback and sorted out the CI failures @jelbourn. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Caretaker note: removing the prototype operators can potentially break some apps on sync |
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly and chaining them via the `RxChain` class. Fixes angular#2622.
With angular#5314 a lot of `@docs-private` tags that are ignored have been added. Dgeni only includes exports of files if they are explicitly exported in a `src/XXX/index.ts` file. Since this is not the case the `@docs-private` tags can be removed because the Rx chain files are not re-exported anywhere.
With #5314 a lot of `@docs-private` tags that are ignored have been added. Dgeni only includes exports of files if they are explicitly exported in a `src/XXX/index.ts` file. Since this is not the case the `@docs-private` tags can be removed because the Rx chain files are not re-exported anywhere.
For folks who come searching later, these are the operators and observables that are no longer being patched onto your Observable.prototype
|
With angular#5314 a lot of `@docs-private` tags that are ignored have been added. Dgeni only includes exports of files if they are explicitly exported in a `src/XXX/index.ts` file. Since this is not the case the `@docs-private` tags can be removed because the Rx chain files are not re-exported anywhere.
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Refactors the entire codebase not to use the patch operators from RxJS, because they pollute the user's setup. Instead, opts into importing the operators directly and chaining them via the
RxChain
class.Fixes #2622.
Note: I'll add some linting to enforce these changes after they get in.