Pipeable #8976
Pipeable #8976
Conversation
| @@ -1,13 +1,13 @@ | |||
| import {LogEntry, Logger} from './logger'; | |||
| import {ConsoleLoggerStack} from './console-logger-stack'; | |||
| import {NullLogger} from './null-logger'; | |||
| import {toArray} from 'rxjs/operators'; | |||
There was a problem hiding this comment.
This should be an import directly from 'rxjs/operators/toArray'
There was a problem hiding this comment.
no, importing from 'rxjs/operators' is fine
There was a problem hiding this comment.
This is to maintain parity with angular/material2, angular/flex-layout, and angular/preboot, starting with angular/components#8160. It seems like it's an open question though on that thread, and will probably become irrelevant as RxJS 6 comes closer
There was a problem hiding this comment.
Importing from operators does cause a larger bundle currently but from memory that's a bug at the moment and should be fixed. With that in mind I think the more ergonomic approach should be the desired kne
There was a problem hiding this comment.
Please note also that this is a node application not a web application. Bundle size is irrelevant.
There was a problem hiding this comment.
I think it's more about setting a precedent for best practices, as a lot of developers look to first-party packages like this for guidance (node app or no), especially when it comes to RxJS. Again, for this at least it seems to be a non-starter, the only reason I'm leaving this comment up is to archive the discussion.
There was a problem hiding this comment.
Best practices for RxJS 6 is to import {toArray} from 'rxjs'. I'm personally okay with this as it's likely the whole Angular ecosystem will move towards that over the next quarter or two.
|
Maybe blacklist importing the whole rxjs library by adding this to tslint.json |
|
@CDDelta Importing from
Let alone this change is for cli tasks only. |
|
@trotyl I just thought it would be a good idea since that was what @CaerusKaru was pushing for, but you are right this is more of a style thing than a tree-shaking thing. |
| import {Observable} from 'rxjs/Observable'; | ||
|
|
||
| import 'rxjs/add/observable/empty'; | ||
| import { EmptyObservable } from 'rxjs/Observable/EmptyObservable'; |
There was a problem hiding this comment.
Lower case 'o' in rxjs/observable/EmptyObservable.
Also rxjs v6 appears to be moving towards using creation helpers over class instantiation. So may want to go with import { empty } from 'rxjs/observable/empty';
Brocco
force-pushed
the
lettable
branch
2 times, most recently
from
8f264b9 to
9793dcf
Compare
Brocco
force-pushed
the
lettable
branch
6 times, most recently
from
5846f71 to
b5050fa
Compare
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.