fix(router-plugin): resolve infinite redirects and browser hanging #1430

arturovt · 2019-10-31T21:17:45Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/ngxs/store/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?

[x] Bugfix
[ ] 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: #1293 #1407

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

Other information

splincode · 2019-11-01T07:14:23Z

You are the best!!!

packages/router-plugin/src/router.state.ts

markwhitfeld

Great work! Just some small tweaks and questions

markwhitfeld · 2019-11-01T10:50:38Z

integrations/hello-world-ng5/src/tsconfig.app.json

    "baseUrl": "./",
-    "module": "es2015",
-    "types": []
+    "module": "esnext"


Do we want to move away from the default bootstrapped config?

I think we should not touch these configs during this PR. I touched it because dynamic import is a part of the esnext system.

Have we done dynamic imports in the ng5 sample?
Would this be part of typical ng5 code?

It's part of TS itself, not ng5.

packages/router-plugin/src/router.module.ts

packages/router-plugin/src/router.state.ts

markwhitfeld · 2019-11-01T11:19:55Z

I would really like to get this into the v3.6 release!

jest.config.js

tsconfig.json

markwhitfeld · 2019-11-03T05:11:44Z

More changes coming after last night's hectic debugging session!

splincode · 2019-11-10T12:00:24Z

@markwhitfeld please see it, we need it in release

markwhitfeld

Can we restore some of these tests?
Are you happy that the issue is solved? You had some issues after our debugging session... were you able to resolve them?

markwhitfeld · 2019-11-14T13:22:18Z

packages/router-plugin/tests/router.plugin.spec.ts

 } from '../';

 describe('NgxsRouterPlugin', () => {
-  it('should dispatch router state events', fakeAsync(async () => {


Shouldn't we restore this test?

I'm not sure. This test is not explicit and we always have to fix it when we do any change. I don't think it's a good approach.

I think we could restore this test but run expectations on other things, not logs.

This type of test is often called a characterization test. It is not necessarily saying that the behavior is right or wrong but rather just documenting what the current behavior is. The value here is to know what has changed when it does.
You could suffix the test name with "(Characterization)" to make its value explicit.

I will move it to another file for this type of tests to avoid mess.

markwhitfeld · 2019-11-14T13:22:25Z

packages/router-plugin/tests/router.plugin.spec.ts

-    ]);
-  }));
-
-  it('should dispatch router state events for another route', fakeAsync(async () => {


Shouldn't we restore this test?

I'm not sure. This test is not explicit and we always have to fix it when we do any change. I don't think it's a good approach.

I think we could restore this test but run expectations on other things, not logs.

See other comment about Characterization tests

markwhitfeld · 2019-11-14T13:22:58Z

packages/router-plugin/tests/router.plugin.spec.ts

@@ -231,304 +168,6 @@ describe('NgxsRouterPlugin', () => {
    const state = TestBed.get(Store).selectSnapshot(TestState);
    expect(state).toBeTruthy();
    expect(state.url).toEqual('/testpath');
-  }));
-
-  describe('RouterDataResolved', () => {


Shouldn't we restore this fixture? Or has it moved?

This was moved to another file.

Ah, thanks! Did any tests disappear from this fixture in the move?

markwhitfeld · 2019-11-19T18:35:40Z

@arturovt I would love to get this into the next release. Could you make the small tweaks above and add a section about this to the announcement article (as part of this PR) if it is not there yet?

arturovt · 2019-11-19T19:24:25Z

I'm very busy. I will try to find a chance to push those changes at weekends.

markwhitfeld · 2019-11-20T12:11:49Z

@arturovt Should I merge this PR and then you can make the further changes (regarding the tests) on this branch and submit another PR?

arturovt · 2019-11-20T13:39:22Z

As you wish. If you think that those changes, that we've made while debugging, are OK, then you can go with merging.

fix(router-plugin): resolve infinite redirects and browser hanging

63795f3

splincode approved these changes Nov 1, 2019

View reviewed changes

splincode reviewed Nov 1, 2019

View reviewed changes

packages/router-plugin/src/router.state.ts Outdated Show resolved Hide resolved

arturovt mentioned this pull request Nov 1, 2019

feat(router-plugin): implement RouterNavigated action #1432

Closed

3 tasks

markwhitfeld reviewed Nov 1, 2019

View reviewed changes

arturovt added this to the v3.6.0 milestone Nov 1, 2019

arturovt added 3 commits November 1, 2019 19:48

fix: tweaks

b1d37bd

fix: tweaks

48f4d5d

fix: tweaks

8536198

splincode reviewed Nov 2, 2019

View reviewed changes

jest.config.js Outdated Show resolved Hide resolved

splincode reviewed Nov 2, 2019

View reviewed changes

tsconfig.json Outdated Show resolved Hide resolved

fix: tweaks

458c304

splincode requested a review from markwhitfeld November 3, 2019 20:26

splincode approved these changes Nov 3, 2019

View reviewed changes

markwhitfeld reviewed Nov 14, 2019

View reviewed changes

markwhitfeld merged commit 6c6c6b3 into master Nov 20, 2019

arturovt deleted the fix/issues-1293-1407 branch November 20, 2019 20:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(router-plugin): resolve infinite redirects and browser hanging #1430

fix(router-plugin): resolve infinite redirects and browser hanging #1430

arturovt commented Oct 31, 2019

splincode commented Nov 1, 2019

markwhitfeld left a comment

markwhitfeld Nov 1, 2019

arturovt Nov 1, 2019

markwhitfeld Nov 3, 2019

arturovt Nov 3, 2019

markwhitfeld commented Nov 1, 2019

markwhitfeld commented Nov 3, 2019

splincode commented Nov 10, 2019

markwhitfeld left a comment

markwhitfeld Nov 14, 2019

arturovt Nov 14, 2019

arturovt Nov 14, 2019

markwhitfeld Nov 14, 2019

arturovt Nov 14, 2019

markwhitfeld Nov 14, 2019

arturovt Nov 14, 2019

arturovt Nov 14, 2019

markwhitfeld Nov 14, 2019 •

edited

Loading

markwhitfeld Nov 14, 2019

arturovt Nov 14, 2019

markwhitfeld Nov 14, 2019 •

edited

Loading

markwhitfeld commented Nov 19, 2019 •

edited

Loading

arturovt commented Nov 19, 2019

markwhitfeld commented Nov 20, 2019

arturovt commented Nov 20, 2019

fix(router-plugin): resolve infinite redirects and browser hanging #1430

fix(router-plugin): resolve infinite redirects and browser hanging #1430

Conversation

arturovt commented Oct 31, 2019

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

splincode commented Nov 1, 2019

markwhitfeld 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

Choose a reason for hiding this comment

markwhitfeld commented Nov 1, 2019

markwhitfeld commented Nov 3, 2019

splincode commented Nov 10, 2019

markwhitfeld 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

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

Choose a reason for hiding this comment

markwhitfeld Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markwhitfeld Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

markwhitfeld commented Nov 19, 2019 • edited Loading

arturovt commented Nov 19, 2019

markwhitfeld commented Nov 20, 2019

arturovt commented Nov 20, 2019

markwhitfeld Nov 14, 2019 •

edited

Loading

markwhitfeld Nov 14, 2019 •

edited

Loading

markwhitfeld commented Nov 19, 2019 •

edited

Loading