-
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
Create documentation for deprecations and breaking changes and add URLs messages #4776
Comments
See also this issue regarding |
I like the idea. If all agree to this approach I can prepare the docs site for this! |
I had the same problem with startWith and didn't understand the documentation which was |
I think the deprecation messages themselves need to be improved. Things like this keep popping up because the messages aren't sufficiently specific: https://stackoverflow.com/questions/56571406/is-the-startwith-operator-in-rxjs-really-deprecated Also, messages should make sense when presented by a linter - not just when reading the TSDoc. For example, outside of the context of the TSDoc, this might not make much sense:
Actually, those messages aren't too bad. TSLint reports them like this:
|
Yet another: #4918 |
Hi @cartant ! From what I understand there are the following problems:
For 1 I believe this is related to #4841 with precondition of #4688For 2 We can create a script for that. I guess @jwo719 can give details.For 3 we could use formatter:following quick options:
tslint --project tsconfig.json --config tslint.json -t stylish ./index.ts
tslint --project tsconfig.json --config tslint.json -s ./formatters -t stylishRxJS ./index.ts POC can be found here For 4 we could just add another script to the
|
Unfortunately, I don't think we should use a formatter. I was not anticipating a solution requiring developers having to adjust their linting configuration and I don't think we can require them to do that. IMO, it should report understandable messages - that include a URL - without configuration changes. I would prefer to see a message like this:
where The |
I had a look at your answer and a discussion with @jwo719 about the docs part. I would do the following things:
[
{
type: 'deprecation' | 'breaking_change'
name: string,
version: string,
date: Date,
implications?: string,
refactoring?: string,
link?: string (link to deprecation or breaking_change depending on type)
}
]
Where |
@cartant I would like to create a timeline that lists all deprecation by release. What is the best way to discover in which release the deprecation was introduced? |
I went through all deprecations and found the version of the introduction. @benlesh Introducing some rules to commit messages and the changelog will help everyone A LOT! Furthermore, I suggest group similar deprecations within one release. This makes it easier in upgrading versions. |
I created a first draft of the deprecation page. I would need some feedback about the content and the linking. Sometimes it is pretty repetitive content, especially in the remove sections. Also the namings: Removed, Old usage, New usage etc are not really good. I'm happy for every input. :) In the following my first try: ...Deprecations introduced prior to 2018-03-29 (6.0.0-beta.4 )Static method
|
never |
Breaking-Change in version 7.x |
---|---|
Reason | Deprecated because it is more efficient? Some more text here... Some more text here... Some more text here... |
Implications | Replacing never with NEVER |
Usage <= 6.0.0-beta.3
import { never } from 'rxjs';
never();
Usage >= 6.0.0-beta.4
import { NEVER } from 'rxjs';
NEVER;
Static method empty
deprecated in favor of constant EMPTY
empty |
Breaking-Change in version 7.x |
---|---|
Reason | Deprecated because it is more efficient? Some more text here... Some more text here... Some more text here... |
Implications | Replacing empty with EMPTY |
Usage >= 6.0.0-beta.3
import { empty } from 'rxjs';
empty();
Usage >= 6.0.0-beta.4
import { EMPTY } from 'rxjs';
EMPTY;
...
Breaking-changes introduced prior to 2018-03-29 (6.0.0-beta.5)
...
...
Breaking-changes introduced prior to [unknown] (7.x)
Static method never
removed
Deprecated in version 6.0.0.beta-4 see refactoring suggestions there
Static method empty
removed
Deprecated in version 6.0.0.beta-4 see refactoring suggestions there
...
...
I like it. I think it will resolve a lot of confusion. If figuring out when the deprecations were added it too tedious, I'd suggest a section titled "Deprecations introduced prior to /* some date */". |
I updated the draft above and used tables. Thanks for your feedback and time! <3 I guess I have everything from the content side to start. I will discuss the rest with @jwo719 regarding the integration. If more feedback pops up, just shoot! |
@jwo719 I updated the todos with a JSON structure. |
The json structure looks good to me, I could imagine that this could be autogenerated at some point of time! |
Exactly this is the reason I want to start with a JSON right away. :) THX for the answer! |
My understanding is that in v6 and v7 there are going to be API deprecations. TSDoc decprecations have been already been added for some of these and they are causing some confusion.
For example, in this issue users of the package gained the mistaken impression that
of
was deprecated and in this issue, the user was unsure of what was deprecated withstartWith(null)
. In both of these situations, the deprecation messages were due to subtleties with TypeScript's signature matching mechanism.For contributors, it might be obvious what is and what is not deprecated, but it's understandable that users of the package might be confused by deprecation messages that are reported after a patch upgrade - particular when the deprecation message seems to related to 'normal' use.
We could make this somewhat clearer by creating a simple markdown document for each deprecation that lists what is deprecated and why and could add a link to that document in the deprecation message itself. Doing so should not require too much effort and should go some way to curtailing the opening of deprecation-related issues.
@benlesh @jwo719
The text was updated successfully, but these errors were encountered: