-
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
4776 add doc ur ls to deprecation messages #5128
4776 add doc ur ls to deprecation messages #5128
Conversation
` | ||
}, | ||
{ | ||
subject: 'combineLatest', |
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.
Regarding the 'implication', I think it should be an explanation that accompanies the 'before deprecation' and 'after deprecation' snippets. That is, it should explain what's different between the two so that users don't have to try to spot the differences themselves.
Reason
The reason doesn't seem quite right, to me.
My understanding is not that scheduling is going to be released as a separate package, rather that it is only going to be used where appropriate.
ATM, there are signatures all over the place that include optional scheduler parameters.
The means that the implementation needs to check for a scheduler, etc., which references/imports the scheduling and prevents it from being tree-shaken.
Implication
I would say something like:
For
combineLatest
, the removal of thescheduler
parameter means that if callers want notifications to be scheduled, they will have to useobserveOn
.
Before Deprecation
import { combineLatest, asyncScheduler } from "rxjs";
combineLatest(a$, b$, asyncScheduler).subscribe({
next: ([a, b]) => console.log(a, b)
});
After Deprecation
import { combineLatest, asyncScheduler, observeOn } from "rxjs";
combineLatest(a$, b$).pipe(
observeOn(asyncScheduler)
).subscribe({
next: ([a, b]) => console.log(a, b)
});
My thoughts on making the deprecation messages less wordy and easier to interpret in linter output. Whether or not it's worth losing the Also, the linter output really needs to be accompanied by a URL that takes users to the relevant deprecation information in the docs. IMO, code examples, etc. should be moved there and out of the messages. Similarly, I don't think there is any need for the message to say when something is going to be removed, etc. The messages should be as plain and direct as possible, IMO, and should link to the docs. Thoughts below are arranged as:
rxjs/src/internal/observable/never.ts Line 37 in 41888ef
rxjs/src/internal/observable/empty.ts Line 60 in 69f7c6d
rxjs/src/internal/operators/merge.ts Line 37 in 8fbb15e
rxjs/src/internal/operators/zip.ts Line 37 in 8fbb15e
rxjs/src/internal/operators/concat.ts Line 25 in 56cbd22
rxjs/src/internal/observable/zip.ts Line 37 in 56cbd22
rxjs/src/internal/operators/mergeMap.ts Line 16 in 41888ef
rxjs/src/internal/operators/exhaustMap.ts Line 16 in 59b5d82
rxjs/src/internal/operators/switchMap.ts Line 16 in ab49acc
rxjs/src/internal/observable/fromEvent.ts Line 54 in e5ab37d
rxjs/src/internal/observable/forkJoin.ts Line 40 in 7926122
rxjs/src/internal/Scheduler.ts Line 20 in 01a0978
https://github.com/ReactiveX/rxjs/blob/6.0.0-rc.1/src/internal/Observable.ts#L260
https://github.com/ReactiveX/rxjs/blob/6.0.0-rc.1/src/internal/Observable.ts#L265
rxjs/src/internal/operators/race.ts Line 24 in 8f7d7fb
https://github.com/ReactiveX/rxjs/blob/6.4.0/src/internal/Observable.ts#L74
https://github.com/ReactiveX/rxjs/blob/6.4.0/src/internal/Observable.ts#L76
https://github.com/ReactiveX/rxjs/blob/6.4.0/src/internal/Observable.ts#L78
https://github.com/ReactiveX/rxjs/blob/6.4.0/src/internal/operators/tap.ts#L9
https://github.com/ReactiveX/rxjs/blob/6.4.0/src/internal/operators/tap.ts#L11
https://github.com/ReactiveX/rxjs/blob/6.4.0/src/internal/operators/tap.ts#L13
https://github.com/ReactiveX/rxjs/blob/6.4.0/src/internal/Observable.ts#L53
https://github.com/ReactiveX/rxjs/blob/6.4.0/src/internal/operators/timeInterval.ts#L70
https://github.com/ReactiveX/rxjs/blob/6.3.0/src/internal/types.ts#L49
rxjs/src/internal/observable/combineLatest.ts Lines 17 to 30 in 95bd807
rxjs/src/internal/observable/combineLatest.ts Lines 33 to 44 in 95bd807
rxjs/src/internal/observable/combineLatest.ts Lines 47 to 60 in 95bd807
rxjs/src/internal/observable/combineLatest.ts Lines 72 to 89 in 95bd807
rxjs/src/internal/observable/combineLatest.ts Lines 91 to 101 in 95bd807
rxjs/src/internal/operators/partition.ts Line 52 in 6259f20
rxjs/src/internal/observable/concat.ts Lines 7 to 18 in 56cbd22
rxjs/src/internal/observable/merge.ts Lines 8 to 31 in b5a2ac9
rxjs/src/internal/observable/of.ts Lines 9 to 31 in 56cbd22
rxjs/src/internal/operators/startWith.ts Lines 7 to 20 in b5a2ac9
rxjs/src/internal/operators/endWith.ts Lines 7 to 29 in 986be2f
rxjs/src/internal/observable/from.ts Line 7 in b5a2ac9
rxjs/src/internal/observable/of.ts Line 35 in 56cbd22
rxjs/src/internal/observable/forkJoin.ts Lines 12 to 23 in 7926122
rxjs/src/internal/observable/concat.ts Lines 7 to 17 in 56cbd22
rxjs/src/internal/Notification.ts Line 9 in fc9186c
rxjs/src/internal/observable/generate.ts Line 68 in ba8a95f
|
I changed the sentence: I did it because the refactoring is different for xMapTo operators and we need to use other operators. |
@cartant I would need some help with the listed messages.
Please provide information for entries marked with TODO, answer entries marked with NOTES: Thanks for your time front off!
|
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 didn't checkout the pr so far because I really struggle with understanding the code at all. It's so bloated by comments. But additionally I think we need to reconsider the approach to tackle this problem. I barely believe one will be able to maintain the code at all. Let's have a chat offline about how to go on with this.
docs_app/src/app/custom-elements/migration-timeline/data-access/migration-item.ts
Outdated
Show resolved
Hide resolved
...c/app/custom-elements/migration-timeline/data-access/migration-timeline-struckture/README.md
Outdated
Show resolved
Hide resolved
...stom-elements/migration-timeline/data-access/migration-timeline-struckture/migration-item.ts
Outdated
Show resolved
Hide resolved
...c/app/custom-elements/migration-timeline/data-access/migration-timeline-struckture/README.md
Outdated
Show resolved
Hide resolved
...stom-elements/migration-timeline/data-access/migration-timeline-struckture/migration-item.ts
Outdated
Show resolved
Hide resolved
Hi @jwo719 ! Thanks you found time to take a look! As this PR is hanging around quite a while without any feedback at all I'm sure a lot of things need to be adopted! I'll update your first feedback and would be happy to get more after the first fixes. |
@jwo719 Resolved all requests. The comment about weird formatting could be discussed to adjust my ide to the standard. I'm happy to get more detailed feedback on the current status |
Todo discussed in the first feedback from @JOW719:
Other things:
|
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.
The code itself looks pretty fine I just requested some subtle changes, but it's not compiling right now so I couldn't test it manually and also I need to test it from an accessibility point of view. Additionally, I didn't saw any test, I'd really appreciate some unit tests
...imeline/components/migration-timeline/missing-information/deprecation-item-form.component.ts
Outdated
Show resolved
Hide resolved
13fc8d9
to
6b92268
Compare
…ssages. first draft of json.
… navigation link in registry
…plete versions i.e. 8.x.x)
…plete versions i.e. 8.x.x)
…der for empty lists
781af61
to
0dc8c2d
Compare
@BioPhoton we talked about this already, this is an amazing pr, it would just be very difficult for us to maintain this properly. |
WORK IN PROGRESS
Related to #4776
Migration-Timeline
The migration timeline is here to support you in the following things:
Core Features
Optional Features (after core features are done)
Other Infos:
Current Progress
UI
Functionality
Data
Code:
Data
Manual work:
Release
Examples for estimated dates:
Deprecation
deprecationMsgCode
5.3.0
6.0.0-alpha.3
6.0.0-alpha.4
6.0.0-beta.4
6.0.0-rc.0
6.0.0-rc.1
6.0.0-tenacious-rc.2
6.0.0-turbo-rc.4
6.0.0-tactical-rc.1
6.1.0
6.2.0
6.2.1
6.3.3
6.4.0
6.5.0
6.5.1
7.0.0-alpha.0
MOVE
Due to folder structure changes from v5 to v6 I have to move several results from here:
https://github.com/search?utf8=%E2%9C%93&q=%5C%40deprecated+repo%3AReactiveX%2Frxjs+created%3A%22%3C2018-04-11%22+repo%3AReactiveX%2Frxjs%2Ftree%2F5.x+path%3A%2Fsrc&type=Code&ref=advsearch&l=&l=
Edge Cases to clarify
First Draft