chore(Platform): Integrate example-app #12

MikeRyanDev · 2017-04-15T17:40:18Z

Migrates the example-app into the monorepo. This makes the example-app a viable target for complete ngrx integration tests. Right now the app mostly works. Things that block this PR:

@ngrx/router-store breaks badly when instrumented with the Redux Devtools Extension. This is because the router's state is not completely serializable cc @vsavkin
@angular/cli has a hard dependency on TypeScript 2.2 whereas we have a hard min dependency on TypeScript 2.3. Run yarn run clean:ts before trying to start the development server.
Both @ngrx/store and @ngrx/store-devtools now have mixed eager providers making it hard to debug undefined injector errors, see Eager providers and Injector.get() can cause errors angular/angular#15501
Example app unit tests use @ngrx/effects/testing which I do not plan to carry over for @ngrx/effects v4. These should instead be rewritten to use marble diagram tests cc @krjordan

Matmo10 · 2017-04-15T20:25:50Z

example-app/app/app.module.ts

+     * and recalculating new states. This enables powerful time-travel
+     * debugging.
+     *
+     * To use the debugger, install the Redux Devtools extension for either


Is the instrumentOnlyWithExtension() functionality baked into instrument() now?

Yes, there is now just one API for instrumentation regardless of strategy.

coveralls · 2017-04-18T02:10:35Z

Coverage remained the same at 89.791% when pulling cedb2fb on chore/integrate-example-app into 810b6b1 on master.

vsavkin · 2017-04-25T23:30:17Z

@MikeRyanDev the router state is not serializable because it has a cycle. Breaking the cycle would be a breaking change in the router and will have some negative implications on dev ergonomics. Is there a way to insert a function serializing the state?

MikeRyanDev · 2017-04-25T23:35:38Z

@vsavkin Yes, we can write a custom serializer/parser for the dev tools that handles router state in a special way. The interface for these will have to be something like:

function serialize<T>(state: RouterState): T;

function revive<T>(state: T): RouterState;

where T is some shape that can be passed to JSON.stringify().

vsavkin · 2017-05-09T18:25:46Z

@MikeRyanDev I can write one if needed.

Quick question though: Why does it have to be serializable at all?

Matmo10 · 2017-05-12T17:59:12Z

@MikeRyanDev Just FYI (you're probably already aware), deepFreezeing the router state also causes errors.

coveralls · 2017-07-02T02:52:17Z

Coverage decreased (-0.05%) to 88.36% when pulling 246e261 on chore/integrate-example-app into 1f67cb3 on master.

coveralls · 2017-07-02T05:18:27Z

Coverage decreased (-0.05%) to 88.36% when pulling bb4700b on chore/integrate-example-app into 1f67cb3 on master.

brandonroberts · 2017-07-02T17:19:59Z

modules/store/spec/store.spec.ts

@@ -187,7 +187,6 @@ describe('ngRx Store', () => {

      dispatcher.ngOnDestroy();

-      expect(storeSubscription.closed).toBe(true);


@MikeRyanDev this test was failing as the storeSubscription is no longer closed. It didn't show up until I upgraded RxJS. Is this a change in behavior?

Yeah, I'm seeing the same thing. I think this behavior changed in newer versions of rxjs.

coveralls · 2017-07-03T20:42:45Z

Coverage decreased (-0.05%) to 88.446% when pulling ab6ddfd on chore/integrate-example-app into b90df34 on master.

brandonroberts · 2017-07-03T21:53:00Z

@MikeRyanDev this branch is up-to-date and ready for another review

coveralls · 2017-07-03T21:54:39Z

Coverage decreased (-0.05%) to 88.446% when pulling 62b6c01 on chore/integrate-example-app into b90df34 on master.

MikeRyanDev · 2017-07-03T23:38:45Z

example-app/app/books/books.module.ts

+     * from feature modules. Effects can be loaded
+     * eagerly or lazily and will be started immediately.
+     * 
+     * All Effects will only be instantiated regardless of


"instantiated once"

MikeRyanDev · 2017-07-03T23:41:23Z

example-app/app/books/effects/book.spec.ts

+    actions$ = TestBed.get(Actions);
+  });
+
+  describe('search$', () => {


I hope you are prepared to start supporting marble testing questions... 😄

MikeRyanDev · 2017-07-03T23:42:16Z

example-app/app/books/effects/book.ts

+    constructor(
+      private actions$: Actions,
+      private googleBooks: GoogleBooksService,
+      @Optional() @Inject(SEARCH_SCHEDULER) private scheduler: Scheduler,


I would maybe add a comment about why these are necessary for testability purposes.

MikeRyanDev · 2017-07-03T23:47:12Z

example-app/app/reducers/index.ts

+}
+
+/**
+ * The compose function is one of our most handy tools. In basic terms, you give


I don't think many of these comments apply anymore. Maybe add a comment about what createSelector does?

Do we not need the comments for showing how the reducerFactory works?

coveralls · 2017-07-04T02:34:43Z

Coverage decreased (-0.05%) to 88.446% when pulling e8f3bd8 on chore/integrate-example-app into b90df34 on master.

coveralls · 2017-07-04T03:09:42Z

Coverage decreased (-0.05%) to 88.446% when pulling a93862e on chore/integrate-example-app into b90df34 on master.

coveralls · 2017-07-12T02:11:43Z

Coverage decreased (-0.05%) to 88.528% when pulling 47a3d33 on chore/integrate-example-app into d176a11 on master.

coveralls · 2017-07-12T19:32:47Z

Coverage remained the same at 88.426% when pulling 35be189 on chore/integrate-example-app into 68274c9 on master.

coveralls · 2017-07-12T20:00:14Z

Coverage remained the same at 88.426% when pulling af886c5 on chore/integrate-example-app into 68274c9 on master.

sharikovvladislav · 2017-07-12T20:40:51Z

Thank you!

sharikovvladislav · 2017-07-12T21:58:41Z

Is it planned that example app does not work after merging this?

sharikovvladislav · 2017-07-12T22:00:12Z

I get this on ng serve right after yarn install:

WARNING in ./modules/store-devtools/src/devtools.ts
122:523-542 "export 'StoreDevtoolsConfig' was not found in './config'

WARNING in ./modules/store-devtools/src/devtools.ts
122:562-581 "export 'StoreDevtoolsConfig' was not found in './config'

ERROR in Error encountered resolving symbol values statically. Function calls are not supported. Consider replacing the function or lambda with a reference to an exported function (position 47:10 in the original .ts file), resolving symbol compose in /Users/svlad/dev/platform/modules/store/src/utils.ts, resolving symbol developmentReducerFactory in /Users/svlad/dev/platform/example-app/app/reducers/index.ts, resolving symbol AppModule in /Users/svlad/dev/platform/example-app/app/app.module.ts, resolving symbol AppModule in /Users/svlad/dev/platform/example-app/app/app.module.ts, resolving symbol AppModule in /Users/svlad/dev/platform/example-app/app/app.module.ts

macOS 10.12.5

brandonroberts · 2017-07-12T22:01:51Z

@sharikovvladislav it's a known issue with the CLI. If you save any file and let it recompile it will complete successfully

sharikovvladislav · 2017-07-12T22:03:47Z

@brandonroberts
What about warnings? Do they matter?

Ok, if I recompile everything is ok. Lets assume it is not problem. Now I don't have error. But when I open localhost:4200 I have this:

core.es5.js:1020 ERROR Error: Uncaught (in promise): Error: Cannot find module 'app/books/books.module'.
Error: Cannot find module 'app/books/books.module'.
    at webpackEmptyContext (example-app async:2)
    at SystemJsNgModuleLoader.webpackJsonp.../../../core/@angular/core.es5.js.SystemJsNgModuleLoader.loadAndCompile (core.es5.js:5644)
    at SystemJsNgModuleLoader.webpackJsonp.../../../core/@angular/core.es5.js.SystemJsNgModuleLoader.load (core.es5.js:5632)
    at RouterConfigLoader.webpackJsonp.../../../router/@angular/router.es5.js.RouterConfigLoader.loadModuleFactory (router.es5.js:3417)
    at RouterConfigLoader.webpackJsonp.../../../router/@angular/router.es5.js.RouterConfigLoader.load (router.es5.js:3401)
    at MergeMapSubscriber.project (router.es5.js:1569)
    at MergeMapSubscriber.webpackJsonp.../../../../rxjs/operator/mergeMap.js.MergeMapSubscriber._tryNext (mergeMap.js:120)
    at MergeMapSubscriber.webpackJsonp.../../../../rxjs/operator/mergeMap.js.MergeMapSubscriber._next (mergeMap.js:110)
    at MergeMapSubscriber.webpackJsonp.../../../../rxjs/Subscriber.js.Subscriber.next (Subscriber.js:89)
    at ScalarObservable.webpackJsonp.../../../../rxjs/observable/ScalarObservable.js.ScalarObservable._subscribe (ScalarObservable.js:49)
    at webpackEmptyContext (example-app async:2)
    at SystemJsNgModuleLoader.webpackJsonp.../../../core/@angular/core.es5.js.SystemJsNgModuleLoader.loadAndCompile (core.es5.js:5644)
    at SystemJsNgModuleLoader.webpackJsonp.../../../core/@angular/core.es5.js.SystemJsNgModuleLoader.load (core.es5.js:5632)
    at RouterConfigLoader.webpackJsonp.../../../router/@angular/router.es5.js.RouterConfigLoader.loadModuleFactory (router.es5.js:3417)
    at RouterConfigLoader.webpackJsonp.../../../router/@angular/router.es5.js.RouterConfigLoader.load (router.es5.js:3401)
    at MergeMapSubscriber.project (router.es5.js:1569)
    at MergeMapSubscriber.webpackJsonp.../../../../rxjs/operator/mergeMap.js.MergeMapSubscriber._tryNext (mergeMap.js:120)
    at MergeMapSubscriber.webpackJsonp.../../../../rxjs/operator/mergeMap.js.MergeMapSubscriber._next (mergeMap.js:110)
    at MergeMapSubscriber.webpackJsonp.../../../../rxjs/Subscriber.js.Subscriber.next (Subscriber.js:89)
    at ScalarObservable.webpackJsonp.../../../../rxjs/observable/ScalarObservable.js.ScalarObservable._subscribe (ScalarObservable.js:49)
    at resolvePromise (zone.js:770)
    at resolvePromise (zone.js:741)
    at zone.js:818
    at ZoneDelegate.webpackJsonp.../../../../zone.js/dist/zone.js.ZoneDelegate.invokeTask (zone.js:424)
    at Object.onInvokeTask (core.es5.js:3881)
    at ZoneDelegate.webpackJsonp.../../../../zone.js/dist/zone.js.ZoneDelegate.invokeTask (zone.js:423)
    at Zone.webpackJsonp.../../../../zone.js/dist/zone.js.Zone.runTask (zone.js:191)
    at drainMicroTaskQueue (zone.js:584)
    at <anonymous>

Was that planned? =)

brandonroberts · 2017-07-12T22:57:17Z

@sharikovvladislav no 😄 I saw that issue before, but not consistently. Try changing app/books/books.module to ./books/books.module in the routes.ts file and save and see if that works

sharikovvladislav · 2017-07-12T23:02:33Z

@brandonroberts That does not work. Same result. A bit another message (because of path changed): Cannot find module './books/books.module'.

brandonroberts · 2017-07-12T23:03:19Z

Ok. I'll take a look into it

sharikovvladislav · 2017-07-12T23:07:56Z

Note that I run ng serve in root of platform repo. Can I help with something? Do you need more info?

sharikovvladislav · 2017-07-14T19:09:49Z

@brandonroberts I don't know how it is possible, but now everything works fine. I checked in Chrome and Chrome Canary

Some time ago I wanted to make a PR to fix e2e tests. I fixed root problem (that was in ngrx/example-app repo) but now angular application can not be detected in test run.

May be this is related to:

ERROR in Error encountered resolving symbol values statically. Function calls are not supported. Consider replacing the function or lambda with a reference to an exported function (position 47:10 in the original .ts file), resolving symbol compose in /Users/svlad/dev/platform/modules/store/src/utils.ts, resolving symbol developmentReducerFactory in /Users/svlad/dev/platform/example-app/app/reducers/index.ts, resolving symbol AppModule in /Users/svlad/dev/platform/example-app/app/app.module.ts, resolving symbol AppModule in /Users/svlad/dev/platform/example-app/app/app.module.ts, resolving symbol AppModule in /Users/svlad/dev/platform/example-app/app/app.module.ts

What do you think? Do you know issue number in angular-cli?

MikeRyanDev requested a review from brandonroberts April 15, 2017 17:40

Matmo10 reviewed Apr 15, 2017

View reviewed changes

MikeRyanDev force-pushed the chore/integrate-example-app branch 2 times, most recently from 277a201 to cedb2fb Compare April 18, 2017 02:07

Matmo10 mentioned this pull request May 21, 2017

RouteNavigationPayload is not serialization-compliant #27

Closed

brandonroberts force-pushed the chore/integrate-example-app branch from cedb2fb to 3ae33a4 Compare June 30, 2017 02:55

brandonroberts reviewed Jul 2, 2017

View reviewed changes

brandonroberts force-pushed the chore/integrate-example-app branch from bb4700b to ab6ddfd Compare July 3, 2017 20:39

brandonroberts changed the title ~~[WIP] chore: integrate example-app~~ chore(Platform): Integrate example-app Jul 3, 2017

MikeRyanDev commented Jul 3, 2017

View reviewed changes

brandonroberts force-pushed the chore/integrate-example-app branch from a93862e to 47a3d33 Compare July 12, 2017 02:08

brandonroberts force-pushed the chore/integrate-example-app branch from 47a3d33 to 35be189 Compare July 12, 2017 19:21

brandonroberts assigned MikeRyanDev Jul 12, 2017

chore(Example): Integrated example app

af886c5

brandonroberts force-pushed the chore/integrate-example-app branch from 35be189 to af886c5 Compare July 12, 2017 19:48

MikeRyanDev merged commit 22105c1 into master Jul 12, 2017

MikeRyanDev deleted the chore/integrate-example-app branch July 12, 2017 20:26

brandonroberts mentioned this pull request Jul 12, 2017

Real scenario, practical sample app with lazy loading in mind #48

Closed

		@@ -187,7 +187,6 @@ describe('ngRx Store', () => {

		dispatcher.ngOnDestroy();

		expect(storeSubscription.closed).toBe(true);

Uh oh!

chore(Platform): Integrate example-app #12

chore(Platform): Integrate example-app #12

Uh oh!

Conversation

MikeRyanDev commented Apr 15, 2017 • edited by brandonroberts Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Apr 18, 2017

Uh oh!

vsavkin commented Apr 25, 2017

Uh oh!

MikeRyanDev commented Apr 25, 2017

Uh oh!

vsavkin commented May 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Matmo10 commented May 12, 2017

Uh oh!

coveralls commented Jul 2, 2017

Uh oh!

coveralls commented Jul 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jul 3, 2017

Uh oh!

brandonroberts commented Jul 3, 2017

Uh oh!

coveralls commented Jul 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jul 4, 2017

Uh oh!

coveralls commented Jul 4, 2017

Uh oh!

coveralls commented Jul 12, 2017

Uh oh!

coveralls commented Jul 12, 2017

Uh oh!

coveralls commented Jul 12, 2017

Uh oh!

sharikovvladislav commented Jul 12, 2017

Uh oh!

sharikovvladislav commented Jul 12, 2017

Uh oh!

sharikovvladislav commented Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandonroberts commented Jul 12, 2017

Uh oh!

sharikovvladislav commented Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brandonroberts commented Jul 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sharikovvladislav commented Jul 12, 2017

Uh oh!

MikeRyanDev commented Apr 15, 2017 •

edited by brandonroberts

Loading

vsavkin commented May 9, 2017 •

edited

Loading

sharikovvladislav commented Jul 12, 2017 •

edited

Loading

sharikovvladislav commented Jul 12, 2017 •

edited

Loading

brandonroberts commented Jul 12, 2017 •

edited

Loading

sharikovvladislav commented Jul 12, 2017 •

edited

Loading

sharikovvladislav commented Jul 14, 2017 •

edited

Loading