broccolijs / broccoli-persistent-filter

MIT License
12 stars 33 forks source link

`dependencyInvalidation` and `concurrency` should not be required #194

Closed nightire closed 4 years ago

nightire commented 4 years ago

Shall we mark these two options as optional as well?

https://github.com/broccolijs/broccoli-persistent-filter/blob/8e939164bcb95bcbf99b1fcd72dd457dc6e0542f/index.ts#L134-L135

I'm writing a subclass based on the broccoli-persistent-filter in TypeScript, by now, calling super(node, options) in the constructor will cause an unfixable type error:

image

IMO, the dependencyInvalidation, and the concurrency options should not be required because the base class will set a default value for both of them when calling the super(node, option).

Currently, I'm using a workaround below:

interface Options {
  name?: string;
}

interface SuperOptions extends Options {
  dependencyInvalidation: boolean;
  concurrency: number;
}

class MyFilter extends PersistentFilter {
  constructor(node: InputNode, options: Options) {
    options.name = options.name || 'my-filter';
    super(node, options as SuperOptions);
  }
}
nightire commented 4 years ago

If my understanding is correct, I can make a patch PR right away.

rwjblue commented 4 years ago

Ya, I think you are correct.