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: introduce importing document #6548

Merged

Conversation

jakovljevic-mladen
Copy link
Member

Description:
As of RxJS version 7.2.0, a new way of importing operators is introduced. This PR adds a document that covers this addition.

Related issue (if exists):
None

@ladyleet
Copy link
Member

@benlesh any comments on this one?
@niklas-wortmann i saw you looked this over a bit?

@benlesh
Copy link
Member

benlesh commented Oct 20, 2021

@jakovljevic-mladen this is a very detailed document. I think one thing I would like to see if that directly at the top as bold as possible, we state very simply and plainly as possible that as of 7.2, no one should be importing things from rxjs/operators, and should import those things from rxjs instead.

Like, I wouldn't make people scroll or read too far at all before they saw that. It should be bold and obvious.

@jakovljevic-mladen
Copy link
Member Author

jakovljevic-mladen commented Oct 20, 2021

@benlesh

we state very simply and plainly as possible that as of 7.2, no one should be importing things from 'rxjs/operators'

Got it, will add.

How about code examples in the docs? This isn't updated anywhere. I was thinking about adding some PRs to update this, but import from 'rxjs/operators' is still not officially deprecated and is also quite a new addition, so users using lower versions might have issues. Thus, I was thinking to refactor code examples; for example from this:

import { fromEvent } from 'rxjs';
import { filter } from 'rxjs/operators';

to this:

import { fromEvent, filter } from 'rxjs';
// If you're using RxJS v7.1 or lower, use this import
// import { fromEvent } from 'rxjs';
// import { filter } from 'rxjs/operators';

What do you think about doing it this way? Or I should just do this:

import { fromEvent, filter } from 'rxjs';

@benlesh
Copy link
Member

benlesh commented Oct 20, 2021

How about code examples in the docs? This isn't updated anywhere.

Yeah. This needs updated everywhere.

@niklas-wortmann
Copy link
Member

We do have the <span class="informal">CONTENT GOES HERE</span> that we use in several places to give a TLDR. I think that would be good to highlight that you don't need rxjs/operators anymore

Copy link
Member

@niklas-wortmann niklas-wortmann 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 for writing a document about this!

@niklas-wortmann
Copy link
Member

I created a seperated ticket for updating the imports in the code example. I guess it would be the best to do this in a separate PR as it will be quite bloated probably.

@jakovljevic-mladen
Copy link
Member Author

jakovljevic-mladen commented Oct 21, 2021

@benlesh @niklas-wortmann

Updated, it looks like this now:

image

It's not the first section in the document, but it's still visible without scrolling. Is this OK? And, yes, I used <span class="informal">, plus I used bold content.

@niklas-wortmann

I guess it would be the best to do this in a separate PR as it will be quite bloated probably.

I was thinking about doing it in separate PRs, not this one. Thanks for creating an issue for this.

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

4 participants