feat: update angular dependencies to V7 #1354

timdeschryver · 2018-10-06T15:31:13Z

Work in progress do not merge.

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Update to Angular 7.

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

timdeschryver · 2018-10-06T15:33:23Z

modules/entity/spec/sorted_state_adapter.spec.ts

-    Object.defineProperty(Array.prototype, 'unwantedField', {
-      value: undefined,
-    });
+    delete (Array.prototype as any).unwantedField;


Had to do this otherwise the integration router specs were failing.
Could the creation of this property on the array be left out?

TypeError: Cannot assign to read only property 'unwantedField' of object '[object Array]' at new NodeList (C:\Users\tdeschryver\dev\platform\node_modules\domino\lib\NodeList.es6.js:8:44) at Document.value (C:\Users\tdeschryver\dev\platform\node_modules\domino\lib\Document.js:726:33) at DominoAdapter.BrowserDomAdapter.querySelectorAll (C:\Users\tdeschryver\dev\packages\platform-browser\src\browser\browser_adapter.ts:122:66) at DOMTestComponentRenderer.insertRootElement (C:\Users\tdeschryver\packages\platform-browser-dynamic\testing\src\dom_test_component_renderer.ts:25:31) at TestBedViewEngine.createComponent (C:\Users\tdeschryver\packages\core\testing\src\test_bed.ts:615:27)

I think its ok leave it here

timdeschryver · 2018-10-06T15:33:32Z

modules/entity/spec/unsorted_state_adapter.spec.ts

-    Object.defineProperty(Array.prototype, 'unwantedField', {
-      value: undefined,
-    });
+    delete (Array.prototype as any).unwantedField;


timdeschryver · 2018-10-06T15:34:26Z

modules/router-store/spec/integration.spec.ts

@@ -494,7 +494,7 @@ describe('integration spec', () => {
      });
  });

-  it('should support cancellation of initial navigation using canLoad guard', (done: any) => {
+  xit('should support cancellation of initial navigation using canLoad guard', (done: any) => {


This test is failing, I don't know why yet.
(It timeouts because then((r: boolean) => {...}) isn't being invoked)

I believe this is a regression issue - angular/angular#26284

timdeschryver · 2018-10-06T15:35:59Z

modules/router-store/spec/utils.ts

@@ -54,6 +54,13 @@ export function createTestModule(
        provide: 'CanLoadNext',
        useValue: opts.canLoad || (() => true),
      },
+      {
+        provide: Console,


Overriding console logs, otherwise the console will be bloated with warnings that the router.navigate runs outside Angular zone. angular/angular#24959

coveralls · 2018-10-06T15:58:29Z

Coverage decreased (-0.1%) to 88.324% when pulling b036b94 on wip/angular-7 into a9e7cbd on master.

timdeschryver · 2018-10-06T16:00:21Z

While the tests are green of the example app (and builds), there is something wrong with it. When you start it, it's stuck in an infinite loop because of AuthGuard.
I think this has something to do with the failing integration test.

timdeschryver · 2018-10-19T18:49:12Z

I've upgraded the deps to Angular 7.
While the tests are green now, the example app is still stuck in an infinite loop.

brandonroberts · 2018-10-19T19:10:09Z

Same router issue?

timdeschryver · 2018-10-19T19:32:19Z

It's because of the Auth guard.
I don't know for sure if this is releated to the fixed regression issue.

The log prints out:

...
index.ts:50 @ngrx/router-store/cancel
index.ts:50 @ngrx/router-store/request
index.ts:50 @ngrx/router-store/cancel
index.ts:50 @ngrx/router-store/request
index.ts:50 @ngrx/router-store/cancel
index.ts:50 @ngrx/router-store/request
...

brandonroberts · 2018-10-19T20:12:08Z

We'll need to look into that because I think it affects current users also. See #1373

timdeschryver · 2018-10-19T20:22:18Z

I tested the example app without the router-store and it worked.
I would suspect that we'll have to change the router-store implementation to get this fixed.
More specific modified the router event at

platform/modules/router-store/spec/integration.spec.ts

Line 480 in 4c639c6

{ type: 'router', event: 'NavigationStart', url: '/next' },

(modified order of events)

I haven't verified this tho. Will try to do so in the next days.

timdeschryver · 2018-10-20T14:27:10Z

There was an infinite loop because of

platform/modules/router-store/src/router_store_module.ts

Line 205 in 1c10f7d

this.router.navigateByUrl(url).catch(error => {

Because of this we always wanted to redirect to / , we set this url when we are in a NavigationStart event. I'm not sure if there is a better way to prevent this.

The fix in commit 1c10f7d ignores navigation when it's triggered by NavigationStart.

brandonroberts

We'll need to update the docs app dependencies also, but that can be done separately.

brandonroberts · 2018-10-24T14:38:47Z

projects/example-app/src/app/reducers/index.ts

-    console.log('action', action);
-    console.log('next state', result);
-    console.groupEnd();
+    // console.groupCollapsed(action.type);


brandonroberts · 2018-10-24T14:39:28Z

tools/defaults.bzl

@@ -5,8 +5,8 @@ load("@build_bazel_rules_nodejs//:defs.bzl",
     _jasmine_node_test="jasmine_node_test", _npm_package="npm_package")

 DEFAULT_TSCONFIG = "//:tsconfig.json"
-NG_VERSION = "^6.0.0"
-RXJS_VERSION = "^5.6.0-forward-compat.0 || ^6.0.0"
+NG_VERSION = "^7.0.0-rc.0"


Change to ^7.0.0

brandonroberts · 2018-10-24T14:43:04Z

modules/entity/spec/sorted_state_adapter.spec.ts

-    Object.defineProperty(Array.prototype, 'unwantedField', {
-      value: undefined,
-    });
+    delete (Array.prototype as any).unwantedField;


I think its ok leave it here

brandonroberts · 2018-10-24T14:45:19Z

@timdeschryver will you update the commit message for the router-store fix? We may need to cherry pick for a 6.1.x bug fix in the meantime until 7.x is ready.

timdeschryver · 2018-10-24T16:21:33Z

@brandonroberts , is it possible to cherry pick the commits we want to release? Would it make things easier if I would create a separate PR for the router fix, because if that's the case I could do it that way.

brandonroberts · 2018-10-25T13:19:54Z

@timdeschryver yes. I've created a 6.1.x branch you can open a PR against for the router fix. I don't think there any other critical fixes needed for Angular 7 users still on V6.

timdeschryver · 2018-10-25T18:31:08Z

@brandonroberts see #1379.

timdeschryver · 2018-10-26T06:14:10Z

Should I also at the migrations for ng update in this PR? @brandonroberts

brandonroberts · 2018-10-26T13:01:07Z

We don't have any migrations for V7 currently. It should work by using ng update @ngrx/store which we should add to the top of the migration guide.

brandonroberts · 2018-10-26T13:43:36Z

tools/defaults.bzl

-NG_VERSION = "^6.0.0"
-RXJS_VERSION = "^5.6.0-forward-compat.0 || ^6.0.0"
+NG_VERSION = "^7.0.0"
+RXJS_VERSION = "^6.3.0"


Let's leave this pinned to ^6.0.0 instead for rxjs

timdeschryver · 2018-10-26T17:21:46Z

Yea, but won't ng update install ngrx v6? https://github.com/ngrx/platform/blob/master/modules/schematics-core/utility/update.ts#L31

brandonroberts · 2018-10-26T17:33:05Z

Today, yes. Once V7 is published, it will be tagged as latest so it will get picked up without us having to tweak any additional migration code. I believe migrations are only run if you specify the --from and --to ranges.

timdeschryver · 2018-10-26T18:25:43Z

I'm not really sure of it.
It will indeed get picked up as "to update" but it won't be installed because we're writing the new version to the package.json's dependencies.

Bazel depends on the package.json for its dependency graph. ngrx-store-freeze implicitly depends on @ngrx/store, which is a package in this monorepo

dzonatan · 2018-11-02T10:35:26Z

Any ETA of release?

timdeschryver commented Oct 6, 2018

View reviewed changes

timdeschryver force-pushed the wip/angular-7 branch from 1919da1 to a3af33c Compare October 6, 2018 18:21

timdeschryver mentioned this pull request Oct 6, 2018

Regression routerNavigateByUrl + canLoad in 7.0-rc.0 <> 6.1 angular/angular#26284

Closed

brandonroberts requested changes Oct 24, 2018

View reviewed changes

timdeschryver force-pushed the wip/angular-7 branch 4 times, most recently from 1febc09 to e3038ce Compare October 24, 2018 16:58

timdeschryver force-pushed the wip/angular-7 branch from e3038ce to 86c00bd Compare October 25, 2018 18:02

brandonroberts requested changes Oct 26, 2018

View reviewed changes

timdeschryver mentioned this pull request Oct 31, 2018

Angular dependencies could use a version bump to 7 #1391

Closed

timdeschryver added 2 commits October 31, 2018 11:46

feat: update Angular 7

86d829e

fix(RouterStore): don't navigate on NavigationStart router event

8f38d52

timdeschryver and others added 6 commits October 31, 2018 11:46

set rxjs version to 6.0.0

d84312f

update Bazel build to 0.17.1

ac4e5fc

temporarily include ngrx-store-freeze in the repo

513a105

Bazel depends on the package.json for its dependency graph. ngrx-store-freeze implicitly depends on @ngrx/store, which is a package in this monorepo

build fixes

ca8dd20

fix path to local ngrx-store-freeze for testing

ac2e652

limit bazel jobs

bdd1a3d

brandonroberts force-pushed the wip/angular-7 branch from 2bb9123 to bdd1a3d Compare October 31, 2018 16:47

more Bazel jobs

0e97fda

brandonroberts force-pushed the wip/angular-7 branch from c5e7a71 to 9b7a711 Compare October 31, 2018 16:57

bump node/yarn engines for ngrx.io

4b5c442

brandonroberts force-pushed the wip/angular-7 branch from 9b7a711 to 4b5c442 Compare October 31, 2018 16:57

brandonroberts added 2 commits October 31, 2018 12:07

rebuild node-sass for docs

4b58b5a

fix typo

b036b94

brandonroberts approved these changes Oct 31, 2018

View reviewed changes

brandonroberts changed the title ~~WIP: feat: update angular~~ feat: update angular dependencies to V7 Oct 31, 2018

brandonroberts merged commit e6048bd into master Oct 31, 2018

brandonroberts deleted the wip/angular-7 branch October 31, 2018 17:47

brandonroberts mentioned this pull request Oct 31, 2018

Build: Update minimum engine versions for node and yarn #1329 #1340

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update angular dependencies to V7 #1354

feat: update angular dependencies to V7 #1354

timdeschryver commented Oct 6, 2018

timdeschryver Oct 6, 2018

brandonroberts Oct 24, 2018

timdeschryver Oct 6, 2018

timdeschryver Oct 6, 2018 •

edited

timdeschryver Oct 6, 2018

timdeschryver Oct 6, 2018

coveralls commented Oct 6, 2018 •

edited

timdeschryver commented Oct 6, 2018 •

edited

timdeschryver commented Oct 19, 2018 •

edited

brandonroberts commented Oct 19, 2018

timdeschryver commented Oct 19, 2018

brandonroberts commented Oct 19, 2018 •

edited

timdeschryver commented Oct 19, 2018 •

edited

timdeschryver commented Oct 20, 2018 •

edited

brandonroberts left a comment

brandonroberts Oct 24, 2018

brandonroberts Oct 24, 2018

brandonroberts Oct 24, 2018

brandonroberts commented Oct 24, 2018

timdeschryver commented Oct 24, 2018

brandonroberts commented Oct 25, 2018

timdeschryver commented Oct 25, 2018

timdeschryver commented Oct 26, 2018 •

edited

brandonroberts commented Oct 26, 2018

brandonroberts Oct 26, 2018

timdeschryver commented Oct 26, 2018

brandonroberts commented Oct 26, 2018

timdeschryver commented Oct 26, 2018 •

edited

dzonatan commented Nov 2, 2018

feat: update angular dependencies to V7 #1354

feat: update angular dependencies to V7 #1354

Conversation

timdeschryver commented Oct 6, 2018

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timdeschryver Oct 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 6, 2018 • edited

timdeschryver commented Oct 6, 2018 • edited

timdeschryver commented Oct 19, 2018 • edited

brandonroberts commented Oct 19, 2018

timdeschryver commented Oct 19, 2018

brandonroberts commented Oct 19, 2018 • edited

timdeschryver commented Oct 19, 2018 • edited

timdeschryver commented Oct 20, 2018 • edited

brandonroberts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonroberts commented Oct 24, 2018

timdeschryver commented Oct 24, 2018

brandonroberts commented Oct 25, 2018

timdeschryver commented Oct 25, 2018

timdeschryver commented Oct 26, 2018 • edited

brandonroberts commented Oct 26, 2018

Choose a reason for hiding this comment

timdeschryver commented Oct 26, 2018

brandonroberts commented Oct 26, 2018

timdeschryver commented Oct 26, 2018 • edited

dzonatan commented Nov 2, 2018

timdeschryver Oct 6, 2018 •

edited

coveralls commented Oct 6, 2018 •

edited

timdeschryver commented Oct 6, 2018 •

edited

timdeschryver commented Oct 19, 2018 •

edited

brandonroberts commented Oct 19, 2018 •

edited

timdeschryver commented Oct 19, 2018 •

edited

timdeschryver commented Oct 20, 2018 •

edited

timdeschryver commented Oct 26, 2018 •

edited

timdeschryver commented Oct 26, 2018 •

edited