Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(schematics): Add ng-add support with prompt for making our schem… #1552

Merged

Conversation

itayod
Copy link
Contributor

@itayod itayod commented Feb 15, 2019

PR Checklist

Please check if your PR fulfills the following requirements:

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:

Closes #1538

What is the new behavior?

Running ng-add will prompt the user to set @ngrx/schematics as the default collection.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@ngrxbot
Copy link
Collaborator

ngrxbot commented Feb 15, 2019

Preview docs changes for 6929ff4 at https://previews.ngrx.io/pr1552-6929ff4/

@itayod
Copy link
Contributor Author

itayod commented Feb 15, 2019

@brandonroberts I hope that's what you had in mind :)

@brandonroberts
Copy link
Member

@itayod thanks for working on this! What I had in mind is that this would be adding an ng-add to the @ngrx/schematics package similar to what the other packages have with a schematics/ng-add folder and a collection.

@itayod
Copy link
Contributor Author

itayod commented Feb 15, 2019

With pleasure! So just to make things clear, you want me to add ng-add to modules/schematics?
Also, does the current functionality of setting @ngrx/schematics as default looks ok?

@brandonroberts
Copy link
Member

Yes and yes

@itayod itayod force-pushed the feature/support-default-schematics-prompt branch from 6929ff4 to 20576a5 Compare February 15, 2019 17:00
@ngrxbot
Copy link
Collaborator

ngrxbot commented Feb 15, 2019

Preview docs changes for 20576a5 at https://previews.ngrx.io/pr1552-20576a5/

@itayod itayod force-pushed the feature/support-default-schematics-prompt branch from 20576a5 to 0f98d7e Compare February 15, 2019 17:38
@ngrxbot
Copy link
Collaborator

ngrxbot commented Feb 15, 2019

Preview docs changes for 0f98d7e at https://previews.ngrx.io/pr1552-0f98d7e/

@itayod itayod force-pushed the feature/support-default-schematics-prompt branch from 0f98d7e to 0c52347 Compare February 16, 2019 17:54
@coveralls
Copy link

coveralls commented Feb 16, 2019

Coverage Status

Coverage increased (+0.09%) to 89.515% when pulling e2d1f20 on itayod:feature/support-default-schematics-prompt into 8532fb1 on ngrx:master.

@ngrxbot
Copy link
Collaborator

ngrxbot commented Feb 16, 2019

Preview docs changes for 0c52347 at https://previews.ngrx.io/pr1552-0c52347/

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for another PR 🎉

To make this package available we also have to add a BUILD file, you could use the effect package for an example.

After this we have to append the package to our existing schematics BUILD:

packages = [
        "//modules/schematics/migrations:npm_package",
        "//modules/schematics/schematics:npm_package", // this line is added
        "//modules/schematics/schematics-core:npm_package",
    ],

"aliases": ["init"],
"factory": "./src/ng-add",
"schema": "./src/ng-add/schema.json",
"description": "Adds initial setup for Ngrx"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using the following description here?

"Installs the NgRx schematics package"

host.overwrite(path, JSON.stringify(workSpace, null, 2));
}

export function updateWorkspace(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only used within the @ngrx/schematics package.
Wouldn't it be better to define these functions within the package itself?

@@ -0,0 +1,16 @@
{
"$schema": "http://json-schema.org/schema",
"id": "SchematicsNgRxRoot",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a bit weird, but the other packages are using the following convention:

Suggested change
"id": "SchematicsNgRxRoot",
"id": "SchematicsNgRxSchematics",

{
"$schema": "http://json-schema.org/schema",
"id": "SchematicsNgRxRoot",
"title": "NgRx Root Schema",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"title": "NgRx Root Schema",
"title": "Scaffolding library for Angular applications using NgRx libraries",

"setSchematicsDefault": {
"type": "boolean",
"default": true,
"description": "Make @ngrx/schematics as the default schematics",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "Make @ngrx/schematics as the default schematics",
"description": "Use @ngrx/schematics as the default collection",

"title": "NgRx Root Schema",
"type": "object",
"properties": {
"setSchematicsDefault": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about using just default or defaultCollection here?

@@ -0,0 +1,3 @@
export interface Schema {
setSchematicsDefault?: boolean;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

"default": true,
"description": "Make @ngrx/schematics as the default schematics",
"x-prompt":
"Would you like to make @ngrx/schematics as the project's default?"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"Would you like to make @ngrx/schematics as the project's default?"
"Would you want to use @ngrx/schematics as the default collection?"

modules/schematics/src/ng-add/index.spec.ts Show resolved Hide resolved
"title": "NgRx Root Schema",
"type": "object",
"properties": {
"setSchematicsDefault": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about adding an alias?

"aliases": ["d"]

@timdeschryver
Copy link
Member

timdeschryver commented Feb 17, 2019

In order to not forget about it, we should also install the @ngrx/schematics package.

@itayod
Copy link
Contributor Author

itayod commented Feb 17, 2019

In order to not forget about it, we should also install the @ngrx/schematics package.

That's what I thought 😄

@brandonroberts
Copy link
Member

We don't need to explicitly install the @ngrx/schematics package into the package.json anymore. You can do ng add @ngrx/schematics today and it will still add it to the project, but will fail when trying to run an ng-add schematic for it.

@itayod
Copy link
Contributor Author

itayod commented Feb 18, 2019

We don't need to explicitly install the @ngrx/schematics package into the package.json anymore. You can do ng add @ngrx/schematics today and it will still add it to the project, but will fail when trying to run an ng-add schematic for it.

Gtn 😄 , I also was thinking about update the schematics settings in the angular.json.
Since most of the case would probably be replacing @schemtics/angular with @ngrx/schematics and since @ngrx/schematics extends @schematics/angular maybe we should also add something like:

  workspaceString.replace(/@scehmatics\/angular/g, '@ngrx/schematics');

So all of the user's configuration for @schematics/angular will apply to @ngrx/schematics,
Thoughts?

@brandonroberts
Copy link
Member

You don't need to replace those. It will use NgRx schematics by default if the collection is set. We may do something with the defaults later, but that varies on the developer's project.

noop,
} from '@angular-devkit/schematics';
import { updateWorkspace } from '../../schematics-core/utility/config';
import { Schema as RootStoreOptions } from './schema';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import { Schema as RootStoreOptions } from './schema';
import { Schema as SchematicOptions } from './schema';

} from '@angular-devkit/schematics/testing';
import * as path from 'path';
import { createWorkspace } from '../../../schematics-core/testing';
import { Schema as RootStoreOptions } from './schema';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
import { Schema as RootStoreOptions } from './schema';
import { Schema as SchematicOptions } from './schema';

@itayod itayod force-pushed the feature/support-default-schematics-prompt branch from 0c52347 to 4cc081b Compare February 19, 2019 20:17
@ngrxbot
Copy link
Collaborator

ngrxbot commented Feb 20, 2019

Preview docs changes for 4cc081b at https://previews.ngrx.io/pr1552-4cc081b/

@ngrxbot
Copy link
Collaborator

ngrxbot commented Feb 20, 2019

Preview docs changes for e2d1f20 at https://previews.ngrx.io/pr1552-e2d1f20/

@brandonroberts brandonroberts merged commit 01ff157 into ngrx:master Feb 20, 2019
@brandonroberts
Copy link
Member

Thanks again @itayod 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Schematics: Add ng-add support with prompt for making our schematics default
5 participants