-
Notifications
You must be signed in to change notification settings - Fork 89
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
feat(upgrade): updated all dependencies #51
base: master
Are you sure you want to change the base?
feat(upgrade): updated all dependencies #51
Conversation
Would love to see this supported for Angular 6 |
import { Observable } from 'rxjs/Observable'; | ||
import 'rxjs/add/observable/forkJoin'; | ||
import 'rxjs/add/operator/map'; | ||
import { Observable , forkJoin } from 'rxjs'; |
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.
There should be no space before the comma.
src/ngxerror.directive.ts
Outdated
import 'rxjs/add/operator/map'; | ||
import 'rxjs/add/operator/distinctUntilChanged'; | ||
import 'rxjs/add/observable/combineLatest'; | ||
import { Observable , Subject , Subscription, combineLatest } from 'rxjs'; |
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 same remark. Code style is not correct in multiple fixed places.
src/ngxerror.directive.ts
Outdated
import 'rxjs/add/operator/map'; | ||
import 'rxjs/add/operator/distinctUntilChanged'; | ||
import 'rxjs/add/observable/combineLatest'; | ||
import { Observable, Subject, Subscription, combineLatest } from 'rxjs'; |
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.
There are still spacing issues here (double spaces after commas).
4542dc6
to
be8693c
Compare
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.
Looks good to me with two exceptions:
- I haven't reviewed changes in
webpack.config.js
. - Before merging changes in
circle.yml
will need to be reverted.
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.
Example tested and working ok.
Also packaged and tested against upgraded project.
Anything else required to get this merged in?
Come on guys, just one review and we are ok. News about this? |
Would love v6 support soon :) |
be8693c
to
b7ebd76
Compare
Well, until the PR get approved I published my forked version |
What blocks this? @mgol @paullinney @toddmotto Is there any reason or we just have to wait? |
@tamasfoldi I'm not a maintainer of this module so I can't do anything. |
guys, review? |
@toddmotto could you have a minute to look into it? |
Pleeease 🙏🏻 |
so is this dead? |
Silence here, no any movement? |
Paging @toddmotto! Is this package dead, or...? |
Does this fork include/resolve the |
Yes, I did it |
Some fresh news about angular and rxjs 6 support? |
How can I help? I am getting a "TypeError: rxjs_Observable__WEBPACK_IMPORTED_MODULE_1__.Observable.combineLatest is not a function" |
This problem is because api of rxjs on version 6 is broken with 5. |
Yeah :) I just wanted to fix this issue - but I noticed that ngx-errors is more broken than I thought :( I can't compile on windows. ... no cp / no mkdir -p ... somebody should really fix this. Back here in a few days - i might go with a @hack-hackages/ngx-errors :) |
The ultimateangular.com guys didn't answer my facebook post. Any success with mail or orther channels? @anymaniax can we fix this? |
I tried twitter without success. |
@egandro maybe this can help you https://stackoverflow.com/questions/35980322/cant-find-combinelatest-in-rxjs-5-0. The problem I think is that you have two combineLatest (operator and function) and you don't use the good import for that. |
@anymaniax i'll migrate to your fork, when updating Angular to 6.1.x and beyond this thing breaks like a stick. Besides it's the only package requiring rxjs-compat so i'm leaving to your repo man. All issues, requests, and stuff will be posted over there... be prepared hahaha. I'll help in anything that i can. Greetings. |
I think this must be fixed in the ngx source. I will do that tomorrow - forking from your branch and you create a @hackages :) |
bump |
I give up here. I just copied the 3 files in my project and remove ngx-erros from the dependency list. Thx for your help! Maybe somebody will fix this project some day... |
What are you adding/fixing?
Support for angular 6 & rxjs 6
Have you added tests for your changes?
No
Will this need documentation changes?
No
Does this introduce a breaking change?
No
Other information
There are some warnings when you start the example app for the bundle size and system.import() into @angular/core