feat(schematics): add support for create effect to schematics #1725

itayod · 2019-04-10T16:32:08Z

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
[ ] 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:

Closes #1682

What is the new behavior?

add support for createEffect function in effect schematics

Does this PR introduce a breaking change?

[ ] Yes
[ ] No

itayod · 2019-04-10T16:35:30Z

...chematics/src/effect/files/__name@dasherize@if-flat__/__name@dasherize__.effects.ts.template

@@ -35,4 +35,22 @@ export class <%= classify(name) %>Effects {
 <% } else { %>
  constructor(private actions$: Actions) {}
 <% } %>
+


If there was an option to extend a template that could be a nice solution here @brandonroberts @timdeschryver do you know if it's possible?

Not currently

ngrxbot · 2019-04-10T16:38:04Z

Preview docs changes for 69b6251 at https://previews.ngrx.io/pr1725-69b6251/

brandonroberts · 2019-04-10T16:57:21Z

We'll also need to account for using action creators with the new effect creators. Instead of using *ActionTypes, the action creator is used, and it calls the action creator instead of creating an instance of an action class.

itayod · 2019-04-10T17:22:03Z

We'll also need to account for using action creators with the new effect creators. Instead of using *ActionTypes, the action creator is used, and it calls the action creator instead of creating an instance of an action class.

Hmm I see, I will try to find a proper solution that will consider the action creator option possibility,
I wounder if there is some way of splicing the template... 🤔

itayod · 2019-04-10T18:23:20Z

@brandonroberts I have updated the solution what do you think?

ngrxbot · 2019-04-10T18:26:25Z

Preview docs changes for ac45157 at https://previews.ngrx.io/pr1725-ac45157/

timdeschryver

The CI build seems to be failing (as an fyi in case you missed it)

modules/schematics/src/effect/index.spec.ts

modules/schematics/src/effect/index.ts

modules/schematics/src/effect/schema.json

modules/schematics/src/feature/schema.json

brandonroberts · 2019-04-10T19:00:06Z

@itayod it would be easier to generate parts of the template in the factory code and pass a variable to the template for output. That's probably as close as we'll get to partial templates.

itayod · 2019-04-10T19:14:06Z

@itayod it would be easier to generate parts of the template in the factory code and pass a variable to the template for output. That's probably as close as we'll get to partial templates.

I agree that's what I just did 😄

brandonroberts · 2019-04-10T20:41:19Z

We can save some line breaks in the template by moving the if cases up. It was too much to type separately.

import { Injectable } from '@angular/core';
import { Actions, <% if (effectCreator) { %>createEffect<% } else { %>Effect<% } %><% if (feature) { %>, ofType<% } %> } from '@ngrx/effects';
<% if (feature && api) { %>import { catchError, map, concatMap } from 'rxjs/operators';
import { EMPTY, of } from 'rxjs';
import { Load<%= classify(name) %>sFailure, Load<%= classify(name) %>sSuccess, <%= classify(name) %>ActionTypes, <%= classify(name) %>Actions } from '<%= featurePath(group, flat, "actions", dasherize(name)) %><%= dasherize(name) %>.actions';
<% } %>
<% if (feature && !api) { %>import { concatMap } from 'rxjs/operators';
import { EMPTY } from 'rxjs';
import { <%= classify(name) %>ActionTypes, <%= classify(name) %>Actions } from '<%= featurePath(group, flat, "actions", dasherize(name)) %><%= dasherize(name) %>.actions';
<% } %>

@Injectable()
export class <%= classify(name) %>Effects {<% if (feature && api) { %>
<% if (!effectCreator) { %>@Effect()<% }%>
  <%= effectMethod %>
    ofType(<%= classify(name) %>ActionTypes.Load<%= classify(name) %>s),
    concatMap(() =>
      /** An EMPTY observable only emits completion. Replace with your own observable API request */
      EMPTY.pipe(
        map(data => new Load<%= classify(name) %>sSuccess({ data })),
        catchError(error => of(new Load<%= classify(name) %>sFailure({ error }))))
    )
  )<% if (effectCreator) { %>)<% }%>;
<% } %><% if (feature && !api) { %>
  <% if (!effectCreator) { %>@Effect()<% }%>
  <%= effectMethod %>
    ofType(<%= classify(name) %>ActionTypes.Load<%= classify(name) %>s),
    /** An EMPTY observable only emits completion. Replace with your own observable API request */
    concatMap(() => EMPTY)
  )<% if (effectCreator) { %>)<% }%>;
<% } %>
<% if (feature) { %>  constructor(private actions$: Actions<<%= classify(name) %>Actions>) {}<% } else { %>  constructor(private actions$: Actions) {}<% } %>
}

ngrxbot · 2019-04-10T22:54:49Z

Preview docs changes for 80ece49 at https://previews.ngrx.io/pr1725-80ece49/

ngrxbot · 2019-04-11T15:23:01Z

Preview docs changes for 137eb71 at https://previews.ngrx.io/pr1725-137eb71/

itayod · 2019-04-11T15:28:52Z

I have updated the code.
just one nit, what do you think about using ts compiler instead regex for the tests?

ngrxbot · 2019-04-11T15:31:43Z

Preview docs changes for c0f85cd at https://previews.ngrx.io/pr1725-c0f85cd/

timdeschryver

LGTM

timdeschryver

I was a tidbit too fast 😅

Should we also add a test to check createEffect isn't used when we don't use the -c flag?

timdeschryver · 2019-04-11T18:00:08Z

modules/schematics/src/effect/index.ts

+  const effectName = stringUtils.classify(name);
+  return effectCreator
+    ? `load${effectName}s$ = createEffect(() => this.actions$.pipe(`
+    : '@Effect()\n' + `\tload${effectName}s$ = this.actions$.pipe(`;


The tab here isn't needed, currently it will output:

@Effect() loadFoos$ = this.actions$.pipe( ofType(FooActionTypes.LoadFoos), /** An EMPTY observable only emits completion. Replace with your own observable API request */ concatMap(() => EMPTY) );

hmm weird when I test it I am getting different result 🤔

apparently because I use tab as 2 spaces in my pc it looked good...
thanks for your notice!! :)

timdeschryver · 2019-04-11T18:01:59Z

just one nit, what do you think about using ts compiler instead regex for the tests?

Not sure what you mean, how would a ts compiler test look like?

modules/schematics/src/effect/schema.json

modules/schematics/src/effect/schema.ts

modules/schematics/src/feature/schema.ts

modules/schematics/src/effect/schema.ts

itayod · 2019-04-11T19:30:58Z

just one nit, what do you think about using ts compiler instead regex for the tests?

Not sure what you mean, how would a ts compiler test look like?

Well, I mean use ts compiler to test if the template has the expected result.
I have created a quick function to demonstrate how:

function getImportSpecifiersFor(file: ts.Node, importName: string) {
  let imports;
  file.forEachChild((node) => {
    if (ts.isImportDeclaration(node)
        && node.moduleSpecifier.text === importName
        && node.importClause) {
      imports = (node.importClause.namedBindings as NamedImports).elements
          .map((importElem) => importElem.name.text)
    }
  })
}

and then the test looks:

    const importsFromNgrx = getImportSpecifiersFor(file, '@ngrx/effects');
    expect(importsFromNgrx).toEqual([ 'Actions', 'createEffect', 'ofType' ]);

ngrxbot · 2019-04-11T20:13:47Z

Preview docs changes for b44e3cc at https://previews.ngrx.io/pr1725-b44e3cc/

brandonroberts · 2019-04-11T20:32:56Z

modules/schematics/src/effect/index.spec.ts

@@ -316,4 +317,42 @@ describe('Effect Schematic', () => {
      /constructor\(private actions\$: Actions<FooActions>\) {}/
    );
  });
+
+  it('should create an effect using creator function', () => {
+    const options = { ...defaultOptions, effectCreator: true, feature: true };


Suggested change

const options = { ...defaultOptions, effectCreator: true, feature: true };

const options = { ...defaultOptions, effectCreators: true, feature: true };

brandonroberts · 2019-04-11T20:33:06Z

modules/schematics/src/effect/index.spec.ts

+  it('should create an api effect using creator function', () => {
+    const options = {
+      ...defaultOptions,
+      effectCreator: true,


Suggested change

effectCreator: true,

effectCreators: true,

brandonroberts · 2019-04-11T20:33:28Z

modules/schematics/src/feature/schema.ts

+  /**
+   * Specifies if the effect creation uses 'createEffect'
+   */
+  effectCreator?: boolean;


Suggested change

effectCreator?: boolean;

effectCreators?: boolean;

ngrxbot · 2019-04-12T14:07:57Z

Preview docs changes for 518f6e9 at https://previews.ngrx.io/pr1725-518f6e9/

itayod commented Apr 10, 2019

View reviewed changes

itayod force-pushed the feature/effect-creator-schematics branch from 69b6251 to ac45157 Compare April 10, 2019 18:19

timdeschryver requested changes Apr 10, 2019

View reviewed changes

itayod force-pushed the feature/effect-creator-schematics branch from ac45157 to 079517c Compare April 10, 2019 19:12

itayod force-pushed the feature/effect-creator-schematics branch 2 times, most recently from b1cecf6 to 80ece49 Compare April 10, 2019 19:17

itayod force-pushed the feature/effect-creator-schematics branch from 80ece49 to 137eb71 Compare April 11, 2019 15:15

itayod force-pushed the feature/effect-creator-schematics branch from 137eb71 to c0f85cd Compare April 11, 2019 15:24

timdeschryver approved these changes Apr 11, 2019

View reviewed changes

timdeschryver reviewed Apr 11, 2019

View reviewed changes

brandonroberts requested changes Apr 11, 2019

View reviewed changes

brandonroberts mentioned this pull request Apr 11, 2019

Schematics: Add schematics for action creators #1670

Closed

itayod force-pushed the feature/effect-creator-schematics branch from c0f85cd to b44e3cc Compare April 11, 2019 20:07

brandonroberts approved these changes Apr 11, 2019

View reviewed changes

feat(schematics): add support for create effect to schematics

518f6e9

itayod force-pushed the feature/effect-creator-schematics branch from b44e3cc to 518f6e9 Compare April 12, 2019 14:02

brandonroberts merged commit 8901abd into ngrx:master Apr 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(schematics): add support for create effect to schematics #1725

feat(schematics): add support for create effect to schematics #1725

itayod commented Apr 10, 2019 •

edited

Loading

itayod Apr 10, 2019

brandonroberts Apr 10, 2019

ngrxbot commented Apr 10, 2019

brandonroberts commented Apr 10, 2019

itayod commented Apr 10, 2019

itayod commented Apr 10, 2019

ngrxbot commented Apr 10, 2019

timdeschryver left a comment

brandonroberts commented Apr 10, 2019

itayod commented Apr 10, 2019

brandonroberts commented Apr 10, 2019

ngrxbot commented Apr 10, 2019

ngrxbot commented Apr 11, 2019

itayod commented Apr 11, 2019

ngrxbot commented Apr 11, 2019

timdeschryver left a comment

timdeschryver left a comment

timdeschryver Apr 11, 2019

itayod Apr 11, 2019

itayod Apr 11, 2019

timdeschryver commented Apr 11, 2019

itayod commented Apr 11, 2019

ngrxbot commented Apr 11, 2019

brandonroberts Apr 11, 2019

brandonroberts Apr 11, 2019

brandonroberts Apr 11, 2019

ngrxbot commented Apr 12, 2019

	const options = { ...defaultOptions, effectCreator: true, feature: true };
	const options = { ...defaultOptions, effectCreators: true, feature: true };

feat(schematics): add support for create effect to schematics #1725

feat(schematics): add support for create effect to schematics #1725

Conversation

itayod commented Apr 10, 2019 • edited Loading

PR Checklist

PR Type

What is the new behavior?

Does this PR introduce a breaking change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngrxbot commented Apr 10, 2019

brandonroberts commented Apr 10, 2019

itayod commented Apr 10, 2019

itayod commented Apr 10, 2019

ngrxbot commented Apr 10, 2019

timdeschryver left a comment

Choose a reason for hiding this comment

brandonroberts commented Apr 10, 2019

itayod commented Apr 10, 2019

brandonroberts commented Apr 10, 2019

ngrxbot commented Apr 10, 2019

ngrxbot commented Apr 11, 2019

itayod commented Apr 11, 2019

ngrxbot commented Apr 11, 2019

timdeschryver left a comment

Choose a reason for hiding this comment

timdeschryver 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

timdeschryver commented Apr 11, 2019

itayod commented Apr 11, 2019

ngrxbot commented Apr 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ngrxbot commented Apr 12, 2019

itayod commented Apr 10, 2019 •

edited

Loading