Skip to content

feat(upgrade): updated all dependencies#51

Open
anymaniax wants to merge 2 commits into
ultimatecourses:masterfrom
hackages:feature/upgrade-dependencies
Open

feat(upgrade): updated all dependencies#51
anymaniax wants to merge 2 commits into
ultimatecourses:masterfrom
hackages:feature/upgrade-dependencies

Conversation

@anymaniax

Copy link
Copy Markdown

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

@craigsmitham

Copy link
Copy Markdown

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';

Copy link
Copy Markdown

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.

Comment thread 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';

Copy link
Copy Markdown

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.

Comment thread 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';

Copy link
Copy Markdown

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).

@anymaniax anymaniax force-pushed the feature/upgrade-dependencies branch from 4542dc6 to be8693c Compare June 12, 2018 12:28

@mgol mgol left a comment

Copy link
Copy Markdown

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:

  1. I haven't reviewed changes in webpack.config.js.
  2. Before merging changes in circle.yml will need to be reverted.

@paullinney paullinney left a comment

Copy link
Copy Markdown

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?

@danieldiazastudillo

Copy link
Copy Markdown

Come on guys, just one review and we are ok. News about this?

@jculverwell

Copy link
Copy Markdown

Would love v6 support soon :)

@anymaniax anymaniax force-pushed the feature/upgrade-dependencies branch from be8693c to b7ebd76 Compare June 24, 2018 20:23
@anymaniax

Copy link
Copy Markdown
Author

Well, until the PR get approved I published my forked version

https://www.npmjs.com/package/@hackages/ngxerrors

@tamasfoldi

tamasfoldi commented Jul 4, 2018

Copy link
Copy Markdown

What blocks this? @mgol @paullinney @toddmotto Is there any reason or we just have to wait?

@mgol

mgol commented Jul 4, 2018

Copy link
Copy Markdown

@tamasfoldi I'm not a maintainer of this module so I can't do anything.

@createbuildship

Copy link
Copy Markdown

guys, review?

@createbuildship

Copy link
Copy Markdown

@toddmotto could you have a minute to look into it?

@Londeren

Copy link
Copy Markdown

Pleeease 🙏🏻

@DmitryEfimenko

Copy link
Copy Markdown

so is this dead?

@dicbrus

dicbrus commented Aug 21, 2018

Copy link
Copy Markdown

Silence here, no any movement?
@anymaniax forked works fine, thanks

@gracegrimwood

Copy link
Copy Markdown

Paging @toddmotto! Is this package dead, or...?

@alignsoft

alignsoft commented Sep 7, 2018

Copy link
Copy Markdown

Does this fork include/resolve the change subject.unsubscribe() to subject.complete() fix in #32?

@anymaniax

Copy link
Copy Markdown
Author

Yes, I did it

@segux

segux commented Sep 14, 2018

Copy link
Copy Markdown

Some fresh news about angular and rxjs 6 support?

@egandro

egandro commented Sep 18, 2018

Copy link
Copy Markdown

How can I help? I am getting a "TypeError: rxjs_Observable__WEBPACK_IMPORTED_MODULE_1__.Observable.combineLatest is not a function"

@segux

segux commented Sep 18, 2018

Copy link
Copy Markdown

How can I help? I am getting a "TypeError: rxjs_Observable__WEBPACK_IMPORTED_MODULE_1__.Observable.combineLatest is not a function"
I Just installed and used Hackages package, he updated with latest rxjs@6 and angular@6

This problem is because api of rxjs on version 6 is broken with 5.

@egandro

egandro commented Sep 18, 2018

Copy link
Copy Markdown

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 :)

@egandro

egandro commented Sep 19, 2018

Copy link
Copy Markdown

The ultimateangular.com guys didn't answer my facebook post. Any success with mail or orther channels? @anymaniax can we fix this?

@DmitryEfimenko

Copy link
Copy Markdown

I tried twitter without success.

@anymaniax

Copy link
Copy Markdown
Author

@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.

@danieldiazastudillo

Copy link
Copy Markdown

@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.

@egandro

egandro commented Sep 24, 2018

Copy link
Copy Markdown

@anymaniax

@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.

I think this must be fixed in the ngx source. I will do that tomorrow - forking from your branch and you create a @hackages :)

@arangates

Copy link
Copy Markdown

bump

@egandro

egandro commented Oct 16, 2018

Copy link
Copy Markdown

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...

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.