Skip to content

feat(upgrade): updated all dependencies#51

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

feat(upgrade): updated all dependencies#51
anymaniax wants to merge 2 commits intoultimatecourses: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.

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.

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
Copy link
Copy Markdown

@mgol mgol left a 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:

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

Copy link
Copy Markdown

@paullinney paullinney left a 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?

@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
Copy link
Copy Markdown

tamasfoldi commented Jul 4, 2018

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

@mgol
Copy link
Copy Markdown

mgol commented Jul 4, 2018

@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
Copy link
Copy Markdown

dicbrus commented Aug 21, 2018

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

@gracegrimwood
Copy link
Copy Markdown

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

@alignsoft
Copy link
Copy Markdown

alignsoft commented Sep 7, 2018

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
Copy link
Copy Markdown

segux commented Sep 14, 2018

Some fresh news about angular and rxjs 6 support?

@egandro
Copy link
Copy Markdown

egandro commented Sep 18, 2018

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

@segux
Copy link
Copy Markdown

segux commented Sep 18, 2018

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
Copy link
Copy Markdown

egandro commented Sep 18, 2018

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
Copy link
Copy Markdown

egandro commented Sep 19, 2018

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
Copy link
Copy Markdown

egandro commented Sep 24, 2018

@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
Copy link
Copy Markdown

egandro commented Oct 16, 2018

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.