chore(config): introduce computed properties

[mgechev]

Fix #823

[TheDonDope]

Hi!

looking good so far! How does this affect an override in project.config.ts ? For example here: https://github.com/mgechev/angular2-seed/blob/master/tools/config/project.config.ts#L24

I guess this code needs an update too, doesn't it?

[mgechev]

Yeah, that is why the build fails. I'll fix it later today.

[mgechev]

Lets discuss the latest proposed changes since they introduce different syntax for overwriting config properties.

Also, if you have ideas for clean and less verbose definition of the property setters which is going to keep the compiler quite let me know.

[TheDonDope]

Hi! Thanks for the updates :)

I don't know if this is possible in TypeScript but wouldn't it be easer to not provide a set methode in seed.config.ts and instead override the get method in project.config.ts like this?

// project.config.ts

get NPM_DEPENDENCIES: InjectableDependency[] {
   const seedDependencies = super.NPM_DEPENDENCIES;
   return seedDependencies.concat(additional_deps);
}

I do not know if it is possible to override a get method this way, but that would have been my initial naive approach to this.

Update: Related TypeScript Issue: microsoft/TypeScript#338 and merged PR microsoft/TypeScript#5860 (Seems to be only possible when emitting to es6)

[mgechev]

Since we are using ts-node for gulpfile.ts and tsconfig which is independent from the project build config we can eventually have this. Unfortunately we need to drop all versions of node that don't support es6...
May be we can stick to this approach and improve it a bit.

[Shyam-Chen]

add whitespace?

import { Overwrite } from '../utils/seed/overwrite_config';

[mgechev]

Fixed.

[TheDonDope]

Hey @mgechev, do you know which node versions would be affected (in terms of needing to be dropped because they do not support es6)? Anything < 5.x?
I am a bit on the fence here, though i can certainly understand the desire to deliver backwards compatibility.
The PR definitely solves the overriding problem, but the code to fix this feels a little verbose (the need to provide the set methods to keep the compiler quiet). This (#968 (comment)) feels a bit cleaner. But i guess we cannot have both (at least not right now).

To sum it up: For me the PR looks fine to merge, even though i wished for something a bit sleeker :)

Thank you @mgechev for tackling the issue! 🎉

[mgechev]

Lets think of a better solution. I'm not huge fen of the one above tool.

[TheDonDope]

One alternate solution i could think of would be to rewrite the properties to actual methods, like this:

before:

  get PORT() {
    return argv['port'] || 5555;
  }

after:

  getPort() {
    return argv['port'] || 5555;
  }

Pros:

• No need for for a set method
• No need for a @Overwrite annotation
• project.config.ts can easily override the methods, while still being able to access the super (seed.config.ts) implementation

Cons:

• More work to rewrite the current code (Code that uses the current properties)

Thoughts on that?

[ludohenin]

I was also thinking that we could wrap the computed config members into a init method and call that one at the end of project.config constructor.

class SeedConfig {
  STATIC_MEMBER = 'blah';
  //...
  init() {
    this.COMPUTED_MEMBER =  `${this.STATIC_MEMBER}/blah`;
  }
}

Pros

• easy to refactor
• no breaking changes (I think)

Cons

• Overriding computed members will have to be done after the init() is invoked (convention)
• ...

[mgechev]

The methods approach seems quite verbose too but we don't have to complicate the configuration by having additional conventions.

The init method approach seems simple. I don't like the convention but it doesn't seem like a big deal to ask people defile property values before init().

The current approach has disadvantage that we have useless setters and we add extra @Overwrite decorator. The decorator doesn't seem like a big deal because we'd have already provided an example. The setters seem annoying. What I like in this approach is that we still have computed properties. We can change value of a base (non-computed) property runtime and everything is still going to work. But the entire approach here seems quite hacky.

I don't love any of these three options.

[mgechev]

I'll think about a better solution over the weekend.

[lukaszpiotrluczak]

@mgechev: Have you had a chance to think about better solution? (some weekends has passed ;) )