Skip to content

Conversation

@ZeevKatz
Copy link

@ZeevKatz ZeevKatz commented Jun 2, 2018

No description provided.

@coveralls
Copy link

coveralls commented Jun 2, 2018

Coverage Status

Coverage increased (+3.9%) to 94.379% when pulling e7c20cc on zeevkatz:master into 8e18cd6 on ghidoz:master.

"rimraf": "^2.6.2",
"rxjs": "5.4.2",
"rxjs": "^6.2.0",
"rxjs-compat": "^6.2.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need rxjs-compat? My understanding ist, that this is only needed for backwards compatibility and the aim of this pull request is to update all rxjs related code to the current version.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there are packages that depend on rxjs<6.0.0 and need rxjs-compat.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Edit: Actually I needed to update my devDependencies as well :/ (angular to ^6)

I just checked it, but it seems the error message only looks like that there is a dependency problem.

The actual problem is, that there is still old rxjs code used:

I fixed both points locally and the servetest script runs through. Does it work for you to?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, additionally I had to update the following devDependencies:

    "@angular/animations": "^6.0.4",
    "@angular/common": "^6.0.4",
    "@angular/compiler": "^6.0.4",
    "@angular/compiler-cli": "^6.0.4",
    "@angular/core": "^6.0.4",
    "@angular/platform-browser": "^6.0.4",
    "@angular/platform-browser-dynamic": "^6.0.4",
    "@angular/platform-server": "^6.0.4",
...
    "codelyzer": "~4.2.1",
....
    "typescript": "2.7.2",
....
    "zone.js": "^0.8.26"

@safo6m: Would it make sense to do this in this pull request (as rxjs 6.x is only usable in angular 6+)?

});

it ('should to rollback to the initial author name', () => {
author.rollbackAttributes();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the authors name be changed before you roll it back? Otherwise you do not test if the author name is really rolled back.

Copy link
Author

@ZeevKatz ZeevKatz Jun 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hpawe01 No, we shouldn't because it's in the same scope and every change we do on the author object affects on every it() in this describe().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, you are right. Sorry, missed that.

private toQueryString: Function = this.datastoreConfig.overrides
&& this.datastoreConfig.overrides.toQueryString ?
this.datastoreConfig.overrides.toQueryString : this._toQueryString;
&& this.datastoreConfig.overrides.toQueryString ?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use a consistent style, so maybe you could undo the removing of the indention here?

@hpawe01 hpawe01 mentioned this pull request Jun 6, 2018
@safo6m safo6m self-requested a review June 6, 2018 11:40
@safo6m safo6m changed the base branch from master to release/6.0.0 August 3, 2018 06:18
@safo6m safo6m changed the base branch from release/6.0.0 to feature/rxjs-upgrade August 3, 2018 06:20
@safo6m safo6m merged commit 2f9a492 into ghidoz:feature/rxjs-upgrade Aug 3, 2018
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.

4 participants