-
Notifications
You must be signed in to change notification settings - Fork 3k
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
chore: update deprecations #6259
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cartant
force-pushed
the
cartant/deprecations
branch
from
April 25, 2021 13:21
1094dd3
to
0ee3f84
Compare
Neither auditTime nor throttleTime are deprecated, so...
niklas-wortmann
approved these changes
Apr 27, 2021
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
benlesh
approved these changes
Apr 27, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
This PR updates the
@deprecation
tags in the TSDoc. Most of the deprecation messages were sufficiently informative, but some were confusing. And, coupled with the false positives that used to be effected, this seemed to confused some folks - well, it resulted in numerous issues being opened.The changes in the PR make the messages more consistent and informative. Where appropriate links to the deprecation docs are provided so that devs can read a more detailed explanation of what's deprecated and what refactoring is necessary.
I've edited the messages in an attempt to optimize them for display in VS Code, as that's where they're going to be read most often, IMO. VS Code collapses whitespace within the message and displays the message as single, wrapped line - with code formatting and proper links to the deprecation details. It leaves the TSDoc
{@link whatever}
tags as they are - which is unfortunate, but we'll just have to live with it, I suppose. I've removed a fair bit of verbosity - 'please', lots of adjectives, etc. - so that the messages are shorter and are less overwhelming when shown as a single line.I've created a
multicasting.md
deprecations document that has refactor examples for all of the multicasting deprecations. They are much easier to read in that format. And there is more context.IMPORTANT: the multicasting deprecations and refactors use an upcoming change to the
connectable
API - it's going to get a config object withconnector
andresetOnDisconnect
properties. This change will be in an upcoming PR (I've discussed this with Ben) - see #6267.Some specific changes:
$$iterator
symboldefer
TSDoc has been updated to use the same type -ObservableInput
- that's used in the actual APIdelayWhen
had a signature that only existed so that a deprecation could be applied. That deprecation was specific to v6. AFAICT, the deprecation was supposed to serve as a warning, but the behaviour about which it was warning has now been changed. I've removed that signature.materialize
deprecation. IMO, we cannot use a deprecation on an operator to warn people about the values it emits. The operator is not deprecated. There are deprecations on the methods ofNotification
, so if people call those methods, they'll see deprecations at the call site. Deprecating an operator that is not going to be removed will be confusing and counterproductive, IMO.sampleTime
'causeauditTime
, et al. are not deprecated. If we really want to deprecate this, we can do it later - after a discussion.multicasting.md
'cause it's too hard to read long lines of 'pretty' code.There are additional deprecations that I think need to be added - as there are several places in the API in which either an array of sources or rest parameters can be passed - but I will include these in a separate PR.
Related issue (if exists): Nope