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

Delete symbols that are marked for deletion in v8 #7170

Merged
merged 15 commits into from Mar 1, 2023

Conversation

demensky
Copy link
Contributor

@demensky demensky commented Jan 25, 2023

I deleted all symbols that were completely marked for deletion. There are no signature removals in this PR!

Separately, I want to clarify that refCount is also removed, although @deprecated does not specify the version in which it needs to be removed. I came to this conclusion because the version was specified in ConnectableObservable.

BREAKING CHANGE: The merge operator is no longer available. Use mergeWith.
BREAKING CHANGE: The zip operator is no longer available. Use zipWith.
BREAKING CHANGE: The publishLast operator is no longer available. Use share({ connector: () => new AsyncSubject(), resetOnError: false, resetOnComplete: false, resetOnRefCountZero: false }) or connectable(source$, { connector: () => new AsyncSubject(), resetOnDisconnect: false }). Multicasting.
BREAKING CHANGE: The publishReplay operator is no longer available. Use share({ connector: () => new ReplaySubject(1), resetOnError: false, resetOnComplete: false, resetOnRefCountZero: false }) or connectable(source$, { connector: () => new ReplaySubject(1), resetOnDisconnect: false }) Multicasting.
BREAKING CHANGE: The publishBehavior operator is no longer available. Use share({ connector: () => new BehaviorSubject(0), resetOnError: false, resetOnComplete: false, resetOnRefCountZero: false }) or connectable(source$, { connector: () => new BehaviorSubject(0), resetOnDisconnect: false }). Multicasting.
BREAKING CHANGE: The publish operator is no longer available. Use share({ resetOnError: false, resetOnComplete: false, resetOnRefCountZero: false }) or connectable(source$, { connector: () => new Subject(), resetOnDisconnect: false }). Multicasting.
BREAKING CHANGE: The multicast operator is no longer available. Use share({ connector: () => new Subject() }) or connectable(source$, {connector: () => new Subject() }) Multicasting.
BREAKING CHANGE: The refCount operator is no longer available. Multicasting.
BREAKING CHANGE: The ConnectableObservable observable is no longer available. Multicasting.
BREAKING CHANGE: The race operator is no longer available. Use raceWith.
BREAKING CHANGE: The pluck('foo', 'bar') operator is no longer available. Use map(x => x?.foo?.bar).
BREAKING CHANGE: The pairs function is no longer available. Use from(Object.entries(obj)) instead pairs(obj).

Related issue (if exists): #6367

Copy link
Member

@benlesh benlesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mostly okay. The only thing I'd like to change is to make sure that alternatives for the removed operators are in the BREAKING CHANGE notes. (for example, using share({ connector: () => new AsyncSubject(), resetOnError: false, resetOnComplete: false, resetOnRefCountZero: false }) or connectable(source$, { connector: () => new AsyncSubject() }) instead of publishLast())

To understand why I'd like these changed: This is what I use to generate the CHANGELOG.md file. And it's per commit. :) I don't want to rebase your commits because you'll lose credit for them.

@@ -1,6 +1,6 @@
/** @prettier */
import { Observable, ConnectableObservable, connectable, of, AsyncSubject, BehaviorSubject, ReplaySubject, Subject, merge } from 'rxjs';
import { connect, share, multicast, publish, publishReplay, publishBehavior, publishLast, refCount, repeat, retry } from 'rxjs/operators';
import { connect, share, multicast, publish, publishReplay, publishBehavior, refCount, repeat, retry } from 'rxjs/operators';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: It would be great if while we were changing these we stopped using the rxjs/operators exports and started using the rxjs exports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have already researched this issue. I planned to make a separate PR. We can even include an eslint rule for this:

"@typescript-eslint/no-restricted-imports": [
  "error",
  { "name": "rxjs/operators", "message": "Use 'rxjs'" }
],

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just that side changes add a lot of noise to the diff and make them hard to review.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me. FYI: when I say "nit" I means I'm not going to block a merge on it. Just pointing something out. (As you may have noticed, if it was a "big deal" I'd have just added a commit to fix it).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll probably add the eslint rule to the PR for #5078.

@benlesh
Copy link
Member

benlesh commented Jan 25, 2023

Honestly, I think the links in the BREAKING CHANGE message are fine. Approved.

@demensky demensky marked this pull request as draft January 25, 2023 17:17
@demensky demensky force-pushed the deprecated-prepared-for-removal branch from 77fb8ff to 8a85196 Compare January 25, 2023 17:27
@demensky demensky marked this pull request as ready for review January 25, 2023 17:27
@demensky
Copy link
Contributor Author

I almost had time to update all the commits before your comment. 😁

@demensky demensky force-pushed the deprecated-prepared-for-removal branch from b141f3e to 2ae76c4 Compare January 25, 2023 21:55
@demensky
Copy link
Contributor Author

@benlesh, I also removed the pairs function, which I overlooked.

@jakovljevic-mladen
Copy link
Member

jakovljevic-mladen commented Feb 9, 2023

@demensky, thanks for letting me know about this PR which I completely forgot about. In my #7181, I also tried updating docs while removing the pluck operator. This means that I have updated some files that were referencing removed pluck operator.

If you didn't do it (I haven't entirely reviewed this PR), if you have enough time and you'd like to update docs, please check these files:

  • docs_app/content/guide/operators.md and potentially other files from docs_app/content/guide/ folder that referenced removed stuff - migrations can stay for now, old ones will have to be removed, and the new ones will have to be written in a new guide;
  • docs_app/tools/decision-tree-generator/src/tree.yml: please either update or completely remove references to removed stuff - many removed stuff has been renamed (merge -> mergeWith), so please try using renamed versionsif possible before removing children and labels;
  • remove references from JSDoc comments in other operators/functions (like I have for pluck in map operator).

I'm completely fine with updating the docs in a new commit or PR.

@demensky demensky marked this pull request as draft February 9, 2023 20:26
@demensky demensky force-pushed the deprecated-prepared-for-removal branch from 2ae76c4 to 8002f47 Compare February 9, 2023 20:50
@demensky demensky marked this pull request as ready for review February 9, 2023 20:57
@demensky
Copy link
Contributor Author

demensky commented Feb 9, 2023

@jakovljevic-mladen, I have updated the PR according to your suggestions. In the tree.yml file, changing merge to mergeWith does not make much sense because merge exists as a function.

Copy link
Member

@jakovljevic-mladen jakovljevic-mladen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me, but there are still some docs issues that could be improved. If you go to the docs_app folder, install dependencies (npm i) and run docs script (npm run docs), you may notice that there's a nice utility tool that reports issues with links:

image

If possible, and if it doesn't make too much hassle for you, please try removing those as well.

Also, there's one mention of empty in operators.md file:

[`empty`](/api/index/function/empty)

Can you remove this one as well? But, since you create a new commit per removal, I'm completely OK with those changes to go in another commit/PR.

@demensky demensky force-pushed the deprecated-prepared-for-removal branch 2 times, most recently from 01edb4a to 9f3ce07 Compare February 10, 2023 19:12
@demensky
Copy link
Contributor Author

@jakovljevic-mladen, I've fixed the broken links in the documentation.

Copy link
Member

@jakovljevic-mladen jakovljevic-mladen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks a lot for fixing all suggestions.

@benlesh
Copy link
Member

benlesh commented Feb 17, 2023

Oh no! I came here to merge this but there are conflicts. @demensky can you please fix the conflicts? :)

BREAKING CHANGE: The `merge` operator is no longer available. Use `mergeWith`.
BREAKING CHANGE: The `zip` operator is no longer available. Use `zipWith`.
BREAKING CHANGE: The `publishLast` operator is no longer available. Use `share({ connector: () => new AsyncSubject(), resetOnError: false, resetOnComplete: false, resetOnRefCountZero: false })` or `connectable(source$, { connector: () => new AsyncSubject(), resetOnDisconnect: false })`. [Multicasting](https://rxjs.dev/deprecations/multicasting#publishlast).
BREAKING CHANGE: The `publishReplay` operator is no longer available. Use `share({ connector: () => new ReplaySubject(1), resetOnError: false, resetOnComplete: false, resetOnRefCountZero: false })` or `connectable(source$, { connector: () => new ReplaySubject(1), resetOnDisconnect: false })` [Multicasting](https://rxjs.dev/deprecations/multicasting#publishreplay).
BREAKING CHANGE: The `publishBehavior` operator is no longer available. Use `share({ connector: () => new BehaviorSubject(0), resetOnError: false, resetOnComplete: false, resetOnRefCountZero: false })` or `connectable(source$, { connector: () => new BehaviorSubject(0), resetOnDisconnect: false })`. [Multicasting](https://rxjs.dev/deprecations/multicasting#publishbehavior).
BREAKING CHANGE: The `publish` operator is no longer available. Use `share({ resetOnError: false, resetOnComplete: false, resetOnRefCountZero: false })` or ` connectable(source$, { connector: () => new Subject(), resetOnDisconnect: false })`. [Multicasting](https://rxjs.dev/deprecations/multicasting#publish).
BREAKING CHANGE: The `multicast` operator is no longer available. Use `share({ connector: () => new Subject() })` or `connectable(source$, {connector: () => new Subject() })` [Multicasting](https://rxjs.dev/deprecations/multicasting#multicast).
BREAKING CHANGE: The `refCount` operator is no longer available. [Multicasting](https://rxjs.dev/deprecations/multicasting#refcount).
BREAKING CHANGE: The `ConnectableObservable` observable is no longer available. [Multicasting](https://rxjs.dev/deprecations/multicasting#connectableobservable).
BREAKING CHANGE: The `race` operator is no longer available. Use `raceWith`.
BREAKING CHANGE: The `pluck('foo', 'bar')` operator is no longer available. Use `map(x => x?.foo?.bar)`.
BREAKING CHANGE: The `pairs` function is no longer available. Use `from(Object.entries(obj))` instead `pairs(obj)`.
@demensky demensky force-pushed the deprecated-prepared-for-removal branch from 9f3ce07 to ceedc83 Compare February 17, 2023 20:18
@demensky
Copy link
Contributor Author

@benlesh Done!

@demensky demensky force-pushed the deprecated-prepared-for-removal branch from ceedc83 to 6dddca5 Compare February 17, 2023 23:21
@benlesh benlesh merged commit fc8f1f2 into ReactiveX:master Mar 1, 2023
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

Successfully merging this pull request may close these issues.

None yet

3 participants