feat(Schematics): add success/failure effects/actions to ng generate feature by wesleygrimes · Pull Request #1530 · ngrx/platform

wesleygrimes · 2019-01-25T19:04:45Z

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)
Documentation has 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?

Closes #1529
Should resolve #1524

What is the new behavior?

The PR enhances the default effect generated for feature.

Adds 2 new actions: LoadFeatureSuccess and LoadFeatureFailure
Enhances feature state interface and initialState with data, loading and error properties the
Enhances feature reducer adding switch cases for each action type
- LoadFeature action sets loading to true
- LoadFeatureSuccess action sets loading to false, nulls error property and updates data with payload.
- LoadFeatureFailure action sets loading to false, populates error from payload
Enhances default loadFeatureEffect$ to demonstrate the idea of a request action, and then based on the return of an observable dispatch a success or failure action.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

wesleygrimes · 2019-01-25T19:08:27Z

I'm updating the unit tests on this schematic to match the new code

ngrxbot · 2019-01-25T19:10:50Z

Preview docs changes for 99bc13b at https://previews.ngrx.io/pr1530-99bc13b/

coveralls · 2019-01-25T20:28:04Z

Coverage increased (+0.2%) to 88.977% when pulling 8091938 on wesleygrimes:feature-schematic-feature-add-success-failure-effect into d6cc83c on ngrx:master.

ngrxbot · 2019-01-25T20:31:28Z

Preview docs changes for 674056f at https://previews.ngrx.io/pr1530-674056f/

wesleygrimes · 2019-01-25T20:37:47Z

Looks like the bazel build step failed but it was a timeout issue:

#!/bin/bash -eo pipefail
yarn
yarn install v1.10.1
[1/5] Validating package.json...
[2/5] Resolving packages...
[3/5] Fetching packages...
Too long with no output (exceeded 10m0s)

brandonroberts · 2019-01-25T20:47:14Z

We'll investigate if it keeps timing out. Some initial feedback:

This should be behind an option for a feature (--api maybe??), so its opt-in at least for 7.x.
The current tests should remain as-is, with new tests for verifying it works as intended.
We don't want to make any assumptions on state structure, but provide actions and default returns for the reducers.

ngrxbot · 2019-01-25T20:52:29Z

Preview docs changes for 674056f at https://previews.ngrx.io/pr1530-674056f/

wesleygrimes · 2019-01-25T21:05:00Z

@brandonroberts

We'll investigate if it keeps timing out. Some initial feedback:

This should be behind an option for a feature (--api maybe??), so its opt-in at least for 7.x.

The current tests should remain as-is, with new tests for verifying it works as intended.

We don't want to make any assumptions on state structure, but provide actions and default returns for the reducers.

That all sounds good to me. Just one question regarding tests....

How should I handle a current test that is going to fail now because the import { } from line it's checking has changed to include the new Success and Failure actions? Also the test checking that the effect added is correct will fail now because the effect contents have changed.

… normal. wip

ngrxbot · 2019-01-25T21:18:00Z

Preview docs changes for 4615abf at https://previews.ngrx.io/pr1530-4615abf/

ngrxbot · 2019-01-25T21:26:30Z

Preview docs changes for 7cb5bb2 at https://previews.ngrx.io/pr1530-7cb5bb2/

wesleygrimes · 2019-01-25T21:36:19Z

@brandonroberts regarding tests, nevermind, I get what you're saying, I will create new tests that test the schematics with the new flag enabled. The current tests will be reverted to match exactly as before as they are testing with the flag disabled by default.

Sorry for the confusion.

ngrxbot · 2019-01-25T21:53:58Z

Preview docs changes for 9012cc3 at https://previews.ngrx.io/pr1530-9012cc3/

…f api flag enabled

wesleygrimes · 2019-01-25T22:23:28Z

@brandonroberts latest commit includes...

New flagged added named api we can change this if needed.
Existing tests have been left untouched
Created new tests that tests schematics with the api flag enabled.
Templates have been updated to utilize the new api flag and only define and implement the success and failure actions if the api flag is true.

ngrxbot · 2019-01-25T22:26:04Z

Preview docs changes for 1a7bf87 at https://previews.ngrx.io/pr1530-1a7bf87/

ngrxbot · 2019-01-28T14:52:49Z

Preview docs changes for 006a2bc at https://previews.ngrx.io/pr1530-006a2bc/

ngrxbot · 2019-01-28T16:11:14Z

Preview docs changes for 8c059cb at https://previews.ngrx.io/pr1530-8c059cb/

timdeschryver

LGTM 👍

brandonroberts · 2019-01-28T17:53:15Z

+  load<%= classify(name) %>s$ = this.actions$.pipe(
+    ofType(<%= classify(name) %>ActionTypes.Load<%= classify(name) %>s),
+    concatMap(() =>
+      EMPTY.pipe(


I feel like we need some comment here to tell the user to replace the empty observable?

Suggested change

EMPTY.pipe(

/** An EMPTY observable only emits completion. Replace with your own observable API request */

EMPTY.pipe(

That makes sense to me.

ngrxbot · 2019-01-28T18:33:08Z

Preview docs changes for 8091938 at https://previews.ngrx.io/pr1530-8091938/

brandonroberts · 2019-01-28T19:30:43Z

LGTM. Docs for https://ngrx.io/guide/schematics/feature can be done separately.

feat: add success/failure effects/actions to ng generate feature

99bc13b

feat: add/update unit tests for additional effects for success failure

674056f

wesleygrimes added 2 commits January 25, 2019 16:11

feat: remove added state properties, revert some of the tests back to…

4615abf

… normal. wip

feat: revert effect\index.spec.ts back to normal

7cb5bb2

feat: adding api flag to schematics, cleaning up tests

9012cc3

feat: implement api flag in templates, add new tests for schematics i…

1a7bf87

…f api flag enabled

timdeschryver reviewed Jan 27, 2019

View reviewed changes

timdeschryver requested changes Jan 27, 2019

View reviewed changes

Comment thread modules/schematics/src/effect/files/__name@dasherize@if-flat__/__name@dasherize__.effects.ts Outdated

Comment thread modules/schematics/src/reducer/files/__name@dasherize@if-flat__/__name@dasherize__.reducer.ts

brandonroberts requested changes Jan 28, 2019

View reviewed changes

Comment thread modules/schematics/src/effect/files/__name@dasherize@if-flat__/__name@dasherize__.effects.ts Outdated

Comment thread modules/schematics/src/effect/files/__name@dasherize@if-flat__/__name@dasherize__.effects.ts Outdated

feat: schematics updates for api flag on features

006a2bc

feat: schematics add missing 's' to LoadFoos action type

8c059cb

timdeschryver approved these changes Jan 28, 2019

View reviewed changes

brandonroberts reviewed Jan 28, 2019

View reviewed changes

feat: schematics add comment explaining EMTPY observable

8091938

brandonroberts approved these changes Jan 28, 2019

View reviewed changes

brandonroberts merged commit e17a787 into ngrx:master Jan 28, 2019

This was referenced Feb 20, 2019

Schematics: Default effect leads to an endless loop #1573

Closed

docs(Schematics): add --api #1574

Closed

feeloor mentioned this pull request Feb 23, 2019

docs(Schematics): adding docs for api flag (ngrx/platform#1574) #1577

Merged

3 tasks

	EMPTY.pipe(
	/** An EMPTY observable only emits completion. Replace with your own observable API request */
	EMPTY.pipe(

Uh oh!

Conversation

wesleygrimes commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

wesleygrimes commented Jan 25, 2019

Uh oh!

ngrxbot commented Jan 25, 2019

Uh oh!

coveralls commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngrxbot commented Jan 25, 2019

Uh oh!

wesleygrimes commented Jan 25, 2019

Uh oh!

brandonroberts commented Jan 25, 2019

Uh oh!

ngrxbot commented Jan 25, 2019

Uh oh!

wesleygrimes commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngrxbot commented Jan 25, 2019

Uh oh!

ngrxbot commented Jan 25, 2019

Uh oh!

wesleygrimes commented Jan 25, 2019

Uh oh!

ngrxbot commented Jan 25, 2019

Uh oh!

wesleygrimes commented Jan 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ngrxbot commented Jan 25, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ngrxbot commented Jan 28, 2019

Uh oh!

ngrxbot commented Jan 28, 2019

Uh oh!

timdeschryver left a comment

Choose a reason for hiding this comment

Uh oh!

brandonroberts Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

wesleygrimes Jan 28, 2019

Choose a reason for hiding this comment

Uh oh!

ngrxbot commented Jan 28, 2019

Uh oh!

brandonroberts commented Jan 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

wesleygrimes commented Jan 25, 2019 •

edited

Loading

coveralls commented Jan 25, 2019 •

edited

Loading

wesleygrimes commented Jan 25, 2019 •

edited

Loading

wesleygrimes commented Jan 25, 2019 •

edited

Loading