chore: simplify config, deprecates npm_deps

[mgechev]

Fix #629

Still haven't implemented the setters since they will introduce breaking changes and we need to discuss them. Basically, each individual InjectableDependency already includes env and vendor properties. By default all each dependency belongs to both environments (which could be more that is why is used a dependency array instead of dev, prod and both). Once we intent to get the dependencies using DEPENDENCIES we resolve all of them which belong to npm_modules and merge the result with the ones from APP_ASSETS that belong to the environment.

@ludohenin @d3viant0ne let me know if you find anything wrong.

Also, lets discuss the setters for keeping backwards compatibility.

[joshwiens]

I get what we are doing here but is there a better word than "oldDeps" to convey intent? I can see the future issue questions already. Perhaps "seedDeps" to be consistent in the seed/project theme.

[mgechev]

Yes, seedDeps sounds much better.

[mgechev]

I'll update it tomorrow and assign the issue to you.

[joshwiens]

Outside of me knit picking naming conventions, I think it's a solid change.

I have seen a few people modify the structure of the project.config (another concat iteration) to pull in other technologies. Something to keep in mind, this is a semver Minor but in reality it's a semver Major for a few projects I can think of off the top of my head. We should add a release note of some sort.

[ludohenin]

I don't think it completely fixes #579 but looks good for dependencies.

[mgechev]

@ludohenin yes, it only partially solves it. It mostly deals with #629

[ludohenin]

I've mentioned it once already but I find it cleaner to remove the constructor method (it's useless in our case) and to have project.config like this. Don't you think so ?

export class ProjectConfig extends SeedConfig {
  PROJECT_TASKS_DIR = join(process.cwd(), this.TOOLS_DIR, 'tasks', 'project');

  // APP_TITLE = 'Put name of your app here';
  projectDeps: InjectableDependency[] = [
    // {src: 'jquery/dist/jquery.min.js', inject: 'libs'},
    // {src: 'lodash/lodash.min.js', inject: 'libs'},
  ];

  seedDeps:InjectableDependency[] = this.NPM_DEPENDENCIES;

  NPM_DEPENDENCIES = this.seedDeps.concat(this.projectDeps);

  APP_ASSETS: InjectableDependency[] = [
    // {src: `${this.ASSETS_SRC}/css/toastr.min.css`, inject: true},
    // {src: `${this.APP_DEST}/assets/scss/global.css`, inject: true},
    { src: `${this.ASSETS_SRC}/main.css`, inject: true },
  ];
}

[mgechev]

@ludohenin yes, it looks cleaner. I don't like that seedDeps becomes an instance member.

[ludohenin]

oops, there are conflicts, would you rebase ?

[ludohenin]

@mgechev Yes you're right about instance members. Then we have to keep has it is.

[mgechev]

@ludohenin in the current PR I don't like how we treat the PROD_NPM_DEPENDENCIES, etc. their setters don't do anything and we are not backwards compatible.

[mgechev]

@ludohenin I think it looks better now. Let me know if you find anything wrong.

[ludohenin]

To make ENVIRONMENTS interface "usefull", we may have to move ENVIRONMENTS constant as a class instance member, I suppose. And interface should be moved to seed.config.interfaces.

[ludohenin]

Moving ENVIRONMENTS will allow extensibility.

[ludohenin]

@mgechev have added some comments. Otherwise LGTM.