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

docs: update operator imports in examples #6678

Conversation

jakovljevic-mladen
Copy link
Member

@jakovljevic-mladen jakovljevic-mladen commented Nov 19, 2021

Description:
As of RxJS version 7.2.0, most operators can be imported from 'rxjs' export site. This PR updates code examples in most places. However, content from *.md files from docs_app/content folder is not updated because I'm still waiting for #6548 to be merged. Once that one gets merged, I will update files from docs_app/content.

Also, please forgive me for trying to be consistent and please try to ignore changes such as:

  • removing brackets: (x) -> x
  • removing trailing commas
  • fixing whitespace issues
  • adding missing ;
  • removing H3 headers (###) from examples - most of them are not present in most examples, so I removed almost all remaining ones
  • replacing " with '
  • and so on...

Related issue (if exists):
Closes #6650

@jakovljevic-mladen jakovljevic-mladen marked this pull request as draft Nov 19, 2021
@jakovljevic-mladen jakovljevic-mladen force-pushed the update_operator_imports_in_examples branch from 0688278 to 57280bc Compare Nov 19, 2021
@jakovljevic-mladen jakovljevic-mladen marked this pull request as ready for review Nov 19, 2021
* error: err => console.log(err),
* });
*
* seconds.pipe(timeout(900))
Copy link
Member Author

@jakovljevic-mladen jakovljevic-mladen Nov 19, 2021

Choose a reason for hiding this comment

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

Not sure how timeout came here, so I removed it.

@@ -8,24 +8,23 @@ import { map } from './map';
* The `timestamp` operator maps the *source* observable stream to an object of type
* `{value: T, timestamp: R}`. The properties are generically typed. The `value` property contains the value
* and type of the *source* observable. The `timestamp` is generated by the schedulers `now` function. By
* default it uses the *async* scheduler which simply returns `Date.now()` (milliseconds since 1970/01/01
* default, it uses the `asyncScheduler` which simply returns `Date.now()` (milliseconds since 1970/01/01
Copy link
Member Author

@jakovljevic-mladen jakovljevic-mladen Nov 19, 2021

Choose a reason for hiding this comment

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

I have a fix for this in my other branch. If changes from that branch land to master, asyncScheduler will automatically become a link to the asyncScheduler docs.

@benlesh
Copy link
Member

@benlesh benlesh commented Nov 30, 2021

I'm really sorry, @jakovljevic-mladen, this PR touches so many files in many different ways that It's going to take a very, very long time to review. There seem to be a number of unrelated changes all in one PR, and I'm having a hard time tracking them all mentally during review. (There are >1,400 lines changed and many for different reasons)

Do you think you can break this up into a PR for each type of change? (i.e. changing the docs "Examples"/"Example" headers, or updating the operator imports)

@benlesh
Copy link
Member

@benlesh benlesh commented Nov 30, 2021

I do want to say that I REALLY appreciate this work. It's important, and I'm tempted just to try to review this PR and push it through... but it's because it's so important that I want to make sure I'm able to review it properly and without confusion. If that makes sense.

@jakovljevic-mladen
Copy link
Member Author

@jakovljevic-mladen jakovljevic-mladen commented Nov 30, 2021

No worries, I understand 🙂

I could do multiple PRs, no worries about that. But, if this works better for you: to transform a single commit from this PR to multiple commits, each having it's own purpose - I'd recommend that way though.

Please let me know @benlesh

@jakovljevic-mladen jakovljevic-mladen marked this pull request as draft Dec 2, 2021
@benlesh
Copy link
Member

@benlesh benlesh commented Dec 6, 2021

@jakovljevic-mladen I'm indifferent, but please keep in mind that I'll need to cherry-pick these changes into the 8.x branch. If you think you can do on PR with N commits where each commit consolidates a set of similar changes, that's fine. I just don't want to be in a situation where I'm cherry-picking a ton of individual commits 😅 . I mean, it seems like this could just be 4-5 commits, and that's fine if you want to do that.

@jakovljevic-mladen
Copy link
Member Author

@jakovljevic-mladen jakovljevic-mladen commented Dec 7, 2021

@benlesh

If you think you can do on PR with N commits where each commit consolidates a set of similar changes, that's fine.

Yeah, I can. I already started working on it, should be finished soon. When it's done, doing the review commit-by-commit should be piece of cake 🙂 - as all changes from each commit will be related.

it seems like this could just be 4-5 commits

There may be a bit more than 4-5 commits, but not much more, maybe 6-7 commits in total. And, I guess that they can all be squashed in the end for easier cherry-picking.

@jakovljevic-mladen jakovljevic-mladen force-pushed the update_operator_imports_in_examples branch from 57280bc to fb9a0f7 Compare Dec 7, 2021
@@ -23,8 +23,7 @@ import { map } from './map';
* Emit interval between current value with the last value
*
* ```ts
* import { interval } from "rxjs";
* import { timeInterval, timeout } from "rxjs/operators";
* import { interval, timeInterval } from 'rxjs';
Copy link
Member Author

@jakovljevic-mladen jakovljevic-mladen Dec 7, 2021

Choose a reason for hiding this comment

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

I dismissed seconds.pipe(timeout(900))... example from lines 36 through 40 in one of the later commits, this is why I don't have import for timeout here.

@jakovljevic-mladen
Copy link
Member Author

@jakovljevic-mladen jakovljevic-mladen commented Dec 7, 2021

@benlesh, I managed to split this PR to multiple commits, almost all of them consisting of equally the same set of changes. I'm sorry for this large PR, I really hope this will make it a bit easier to review. Also, I'm sorry, but it it took 11 commits in the end, not 6-7 as I predicted.

Only one commit has some random changes that don't go to any other group, all others should have equally the same set of changes and all of them should be straightforward to review. Here's the list of them and some of my comments:

  • 8b43097e - this is the main commit that consist of the changes required by #6650. What I tried to do here is that I tried to list imported objects by the first appearance in the code example - so, if you see that I changed from e.g. { map, scan } to { scan, map }, this is most probably because map comes after scan in a pipe (e.g. .pipe(scan(...), map(...)). Also, all other imports are sorted this way - by the first appearance.
  • 130d2960 - this commit only updates README.md - I added changes before v7.2 and after v7.2.
  • 27c19b5e - replaced typescript with ts in code example headings - almost every other file in the whole repository uses ts.
  • 99704ee9 - replaced deprecated async with asyncScheduler.
  • 82c27fa0 - fixed headings. Most headings in most operator files use <h2>Example(s)<h2> (markup synonym being ## Example(s)), thus I replaced all H3 headings with H2. Also, most examples don't have dot (.) at the end of sentence, so I removed it from most of those that had it. I fixed whitespacing issues as well - mostly by adding some empty lines between a heading, a description text and a code snippet. Also, if there were many ## Example headings in the same JSDoc, I removed all except the first one (the first one became ## Examples). If some files had a description text prefixed with ###, most files don't have this prefix, so I removed them as well.
  • 0ae39ab8 - fixed links that have this style: {@link something}. This is mostly being removal of the path, e.g. index/interval to just interval. If the {@link } parser can't find a valid link, it will throw an error. I tested all changes from this commit and they all worked. Also, I fixed some @see links, some of them were added, some (duplicates) were removed.
  • d95fed47 - this one is the one that is a total mess. I tried to unify most things, I'm not sure that I managed to do it. Also, I tried to fix some typos and grammar issues. This is a list of some of changes:
    • used single quotes,
    • fixed whitespace issues,
    • deleted trailing commas,
    • removed extra parenthesis,
    • converted to arrow functions,
    • updated parameters to non-deprecated version of subscribe,
    • added missing subscribe call to the final observable from example,
    • removed @prettier JSDoc comments,
    • added missing ;,
    • replaced let with const,
    • removed unused interface Person - TS can now infer data types passed to of/from,
    • fixed some TS issued with event.target by doing casting,
    • replaced errors passed to throwError and throwIfEmpty with arrow functions returning those errors.
  • 7ca231a1 - replaced mapTo and concatMapTo with map and concatMap, respectively, as there is a high chance of those being deprecated one day.
  • 7ebbbfa0 - tap now supports subscribe and finalize callbacks, so I simplified logging in some examples by utilizing those callbacks instead of using defer and/or finalize.
  • df209aff - I simplified share example by introducing tap and take - tap is used to remove console.log call in map and take to be able to document entire output.
  • fb9a0f7e - fix TS error that occurs on this line: const upperCase = materialized.pipe(dematerialize());

I know that there is a huge number of changes unrelated to the main goal which is described in #6650. @benlesh, if you feel like having issues doing the review, please let me know, I can easily drop most commits - I'd leave only the first two as those are only related to #6650. I would cherry pick dropped commits to other branches and I could create separate PRs to address those issues.

@jakovljevic-mladen jakovljevic-mladen marked this pull request as ready for review Dec 7, 2021
@jakovljevic-mladen jakovljevic-mladen requested a review from benlesh Dec 7, 2021
@samal-rasmussen
Copy link
Contributor

@samal-rasmussen samal-rasmussen commented Dec 8, 2021

What an absolutely monumental effort you've made in updating the imports. Bravo. I have one nit with it, which is that alphabetic sorting of import minimizes the amount of git merge conflicts. It's also an easier rule to follow in practice.

@jakovljevic-mladen
Copy link
Member Author

@jakovljevic-mladen jakovljevic-mladen commented Dec 8, 2021

Thanks @samal-rasmussen! Also, thanks for your suggestion. I agree, alphabetic sorting is a better way, but it is usually handled by IDEs when it comes to the real code imports. Since these were JSDocs comments that I had to maintain manually, no IDE could help me with them, so it was much easier for me to keep track of all needed objects (and all unused imports - there were so many of them) than it would if I'd had to sort them alphabetically. Sorting alphabetically would require much more effort as I'd have to think what object comes before another one.

Though, I agree that some existing imports that didn't need to be changed could have been left unchanged, sorry about that.

benlesh
benlesh approved these changes Jan 4, 2022
@@ -39,13 +39,13 @@ export function firstValueFrom<T>(source: Observable<T>): Promise<T>;
* async function execute() {
* const source$ = interval(2000);
* const firstNumber = await firstValueFrom(source$);
* console.log(`The first number is ${firstNumber}`);
* console.log(`The first number is ${ firstNumber }`);
Copy link
Member

@benlesh benlesh Jan 4, 2022

Choose a reason for hiding this comment

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

nit: I personally never type the spaces here, and I've never worked with anyone who did. haha.

* closeObserver: {
* next() {
* const customError = { code: 6666, reason: 'Custom evil reason' }
* console.log(`code: ${ customError.code }, reason: ${ customError.reason }`);
Copy link
Member

@benlesh benlesh Jan 4, 2022

Choose a reason for hiding this comment

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

nit: same as the other.

* const notifA = { kind: 'N', value: 'A' };
* const notifB = { kind: 'N', value: 'B' };
* const notifE = { kind: 'E', error: new TypeError('x.toUpperCase is not a function') }
* const notifA = <NextNotification<string>>{ kind: 'N', value: 'A' };
Copy link
Member

@benlesh benlesh Jan 4, 2022

Choose a reason for hiding this comment

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

Please prefer const notifA: NextNotification<string> = or {} as NextNotification<string>. We're trying to move away from bracket casts.

Copy link
Member

@benlesh benlesh Jan 4, 2022

Choose a reason for hiding this comment

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

This is a "nit". We can fix it later.

Copy link
Member Author

@jakovljevic-mladen jakovljevic-mladen Jan 4, 2022

Choose a reason for hiding this comment

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

Sure, I'll fix in another PR.

@benlesh
Copy link
Member

@benlesh benlesh commented Jan 4, 2022

Thank you so much for all of this work, @jakovljevic-mladen! And thank you for breaking it into different commits. It was much easier to review.

@benlesh benlesh merged commit 888c753 into ReactiveX:master Jan 4, 2022
5 checks passed
@jakovljevic-mladen jakovljevic-mladen deleted the update_operator_imports_in_examples branch Jan 4, 2022
@jakovljevic-mladen
Copy link
Member Author

@jakovljevic-mladen jakovljevic-mladen commented Jan 4, 2022

Thank you @benlesh for having patience to review quite a massive PR.

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.

3 participants