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

RFC: Schematics ng-add implementation #920

Closed
vitaliy-bobrov opened this issue Mar 20, 2018 · 26 comments · Fixed by #1503
Closed

RFC: Schematics ng-add implementation #920

vitaliy-bobrov opened this issue Mar 20, 2018 · 26 comments · Fixed by #1503

Comments

@vitaliy-bobrov
Copy link
Contributor

vitaliy-bobrov commented Mar 20, 2018

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[x ] Feature request
[ ] Documentation issue or request

There is the new command introduced for @angular/cli v6.0.0-beta.5 (and will be part of v6 final release) called ng add. What does it do? It fetches schematic collection from npm registry according to passed collection name and executes ng-add schematic from that collection. It should be used for initial automatic setup of the library inside users project. We could implement such ng-add schematic for @ngrx/schematics package to produce store and feature setup like it described here by calling store and feature schematics.

Flow example:

  1. User runs ng add @ngrx/schematics + additional arguments needed.
  2. Angular CLI runs ng-add schematics from the ngrx collection
  3. Root store, effects, devtools setup in users project automatically

Great article by @manfredsteyer for the reference.

@robwormald
Copy link
Contributor

Yep, all for this.

@vitaliy-bobrov
Copy link
Contributor Author

Great, I'll prepare PR

@MikeRyanDev
Copy link
Member

@robwormald, @brandonroberts: Does it make more sense for the :ng-add schematic to live in each package? That way running ng add @ngrx/store would add Store, ng add @ngrx/effects would add Effects, etc.

@brandonroberts
Copy link
Member

Good question. I think it might be a little confusing if you add store to an application and it gets set up, but you can't generate anything else unless you then install schematics, which would run its initial setup also.

@vitaliy-bobrov
Copy link
Contributor Author

It is possible to use any platform package without the schematic, so IMHO only schematic package should implement ng-add. We can add arguments to specify optional parts generation (StoreDevtoolsModule, Effects).

Another point here that @ngrx/schematics should have some packages as peerDependencies, isn't it?

@robwormald
Copy link
Contributor

+1 to Mike - that’s the right mental model for ng add. Let’s go with this.

@vitaliy-bobrov
Copy link
Contributor Author

Additional question, should ng-add have an alias? And if yes, what alias to use?

@vitaliy-bobrov
Copy link
Contributor Author

@brandonroberts, as per discussion to implement ng-add for each package in the ngrx ecosystem, should we add schematics as a dependency for all of them to re-use generators?

@vitaliy-bobrov
Copy link
Contributor Author

@brandonroberts, could you take a look at PR. For now, I just created ng-add inside store package, as I'm not sure about build process and other stuff, will be great to get right direction ;)

@martzcodes
Copy link
Contributor

So, if every package (e.g. @ngrx/store) has its own ng-add... would this effectively kill @ngrx/schematics since it seems like it would be... redundant?

So ideally a PR would need the ng-add hooks for each package, remove @ngrx/schematics and update the documentation accordingly?

martzcodes added a commit to martzcodes/platform that referenced this issue Apr 28, 2018
@martzcodes
Copy link
Contributor

In that PR I split @ngrx/schematics up into its respective packages which was fairly clean except for the notes below.

Copy / Paste from the PR (which is a WIP):

Right now the feature schematic is broken since it crosses boundaries and I'm not sure on the use of externalSchematic. I also had to keep the modules/schematics because of the strings and utility folder... I didn't want to duplicate those everywhere.

Maybe it could go into the root of the mono-repo?

Open questions:

  • Where to put modules/schematics/src/strings.ts and modules/schematics/src/utility so the packages can all use them?

  • Correct usage of externalSchematic or how to link feature from @ngrx/effects to @ngrx/store?

@martzcodes
Copy link
Contributor

In #999 I moved utilities to root, removed modules/schematics and added ng-adds to the collection.jsons. Seems like they're required even if you don't use them, though that'd be the place to auto-install stuff into app.module.ts or wherever.

feature still doesn't work

Angular Material has some good angular-cli 6 schematics stuff: https://github.com/angular/material2/tree/master/src/lib/schematics

I probably won't touch this too much the rest of the weekend

@brandonroberts
Copy link
Member

Adding support for ng add does not change where @ngrx/schematics fits in. ng add is for initial installation and setup of a library. The other schematics are for building out features.

@martzcodes
Copy link
Contributor

I guess I misinterpreted the comments above. I'll add schematics back, but it seems like @ngrx/schematics could be considered unnecessary if each package already has at least one schematic (ng add). May as well include the schematics that are pertinent to the package being added and have one less dependency / thing to install.

@vitaliy-bobrov
Copy link
Contributor Author

@brandonroberts , should we go in that direction or may be better to keep ng-add in schematics module?
If no, could you give core team vision how to re-use schematics code?

@robwormald
Copy link
Contributor

robwormald commented Apr 28, 2018 via email

@martzcodes
Copy link
Contributor

@vitaliy-bobrov seems like ng-add needs to get added to @ngrx/schematics as a bare minimum (it's required to be there in angular 6 otherwise this gets thrown:

The package that you are trying to add does not support schematics. You can try using a different version of the package or contact the package author to add ng-add support.

It seems like all ng-add typically does is add the package thats being installed into the app's code.

So, this would work...

export default function(options: any): Rule {
  return (host: Tree, context: SchematicContext) => {
    return host;
  };
}

Next step would be to add an ng-add schematic to each package (e.g. @ngrx/store) so when it is installed it automatically gets installed to app.module...

And if you take that a step further...

What I was suggesting, is that if you follow the intent of ng-add and add it to each package, each package would already have at least one schematic... may as well add the appropriate schematics (from @ngrx/schematics) to the respective package so that they're already there and you don't need to install yet another library to do the same thing.

@martzcodes
Copy link
Contributor

Roger that @robwormald

martzcodes added a commit to martzcodes/platform that referenced this issue Apr 29, 2018
@martzcodes
Copy link
Contributor

I think I was mis-understanding some things related to collections/schematics in general, so I closed my PR until I mentally re-group

@brandonroberts
Copy link
Member

Hold on before creating any new PRs. I'll give an update this week

@vitaliy-bobrov
Copy link
Contributor Author

I want to proceed with additional ng-add schematics:

  1. store-devtools
  • optionally add dependency + install
  • add import to the root module
  • arguments to pass into StoreDevtoolsModule.instrument
  1. router-store

@brandonroberts
Copy link
Member

For store-devtools, I think it should be setup the same way it is when you generate the root Store schematic today. Maybe we could add the maxAge to be preset out of the box for performance reasons.

For router-store, it should add the reducer if you provide a reducers argument. I don't want to include router-actions as the are completely optional and were only a migration path from V2 to V4.

@timdeschryver
Copy link
Member

@vitaliy-bobrov Hi, I got a question. I was just toying around with ng add and I noticed something.
Maybe I'm doing it wrong tho...

I created a new project and added @ngrx/store which works. Then I created a new module and tried to add @ngrx/store to it, but I keep getting the error that the module does not exist.
I've tried adding the store via the schematics and here it worked.
What should be the syntax to add the store to a module, or is this working as intended?

ng new banana-app
cd banana-app
ng add @ngrx/store
ng g module strawberry
ng add @ngrx/store --module strawberry/strawberry.module.ts
npm install @ngrx/schematics --save-dev
ng config cli.defaultCollection @ngrx/schematics
ng generate store strawberry/Strawberry --module strawberry.module.ts

@martzcodes
Copy link
Contributor

ng add is for installing libraries, not for linking things together. ng generate store strawberry/Strawberry --module strawberry.module.ts would be the correct way to add a store and tie it into a module

@timdeschryver
Copy link
Member

Yea that is what I thought initially, but I saw it has a module flag.
So I thought it would perhaps be possible 😅 (because perhaps you would want to add the store to a module instead of the AppModule).

@martzcodes
Copy link
Contributor

You had already run ng add @ngrx/store in line 3... I suppose it would work if you ran it again but I don't think that's the recommended way to do things.

¯\(ツ)

I had opened #1143 a few days ago that was similar to this but it had already been fixed in master / hadn't been released yet (I didn't check non-app.module.ts modules though). Try ../strawberry/strawberry.module.ts. If that works it might be worth installing master and trying that to see if there really is a path issue ( which #1144 ) might have fixed.

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

Successfully merging a pull request may close this issue.

6 participants