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: derive constructor options from chainable Base.defaults() calls #57

Merged
merged 19 commits into from
Jul 17, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ const instance = new BaseWithOptions();
instance.options; // {foo: 'bar'}
```

### Defaults

TypeScript will not complain when chaining `.defaults()` calls endlessly: the static `.defaultOptions` property will be set correctly. However, when instantiating from a class with 4+ chained `.defaults()` calls, then only the defaults from the first 3 calls are supported. See [#57](https://github.com/gr2m/javascript-plugin-architecture-with-typescript-definitions/pull/57) for details.

## Credit

This plugin architecture was extracted from [`@octokit/core`](https://github.com/octokit/core.js). The implementation was made possible by help from [@karol-majewski](https://github.com/karol-majewski), [@dragomirtitian](https://github.com/dragomirtitian), and [StackOverflow user "hackape"](https://stackoverflow.com/a/58706699/206879).
Expand Down
115 changes: 93 additions & 22 deletions index.d.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
export namespace Base {
export declare namespace Base {
interface Options {
version: string;
[key: string]: unknown;
}
}
Expand Down Expand Up @@ -30,33 +31,103 @@ declare type ReturnTypeOf<T extends AnyFunction | AnyFunction[]> =
? UnionToIntersection<Exclude<ReturnType<T[number]>, void>>
: never;

type ClassWithPlugins = Constructor<any> & {
plugins: any[];
};

type ConstructorRequiringVersion<Class extends ClassWithPlugins, PredefinedOptions> = {
defaultOptions: PredefinedOptions;
} & (PredefinedOptions extends { version: string }
Comment on lines +38 to +40
Copy link
Owner Author

Choose a reason for hiding this comment

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

It's okay to hard-code the required version right now, but we should make this flexible later.

The Base.Options interface can be extended with other required options, and these should be taken into account. Also the version is random and I just put it in here for exploring the blockers I ran into with Octokit, I don't think it makes sense to have any default options in the Base class, they should all be added when using Base by extending the Base.Options interface

I'll create a follow up issue, I think this PR is big enough as-is 😁

? {
new <NowProvided>(options?: NowProvided): Class & {
options: NowProvided & PredefinedOptions;
};
}
: {
new <NowProvided>(options: Base.Options & NowProvided): Class & {
options: NowProvided & PredefinedOptions;
};
});

export declare class Base<TOptions extends Base.Options = Base.Options> {
static plugins: Plugin[];

/**
* Pass one or multiple plugin functions to extend the `Base` class.
* The instance of the new class will be extended with any keys returned by the passed plugins.
* Pass one argument per plugin function.
*
* ```js
* export function helloWorld() {
* return {
* helloWorld () {
* console.log('Hello world!');
* }
* };
* }
*
* const MyBase = Base.plugin(helloWorld);
* const base = new MyBase();
* base.helloWorld(); // `base.helloWorld` is typed as function
* ```
*/
static plugin<
S extends Constructor<any> & {
plugins: any[];
},
T1 extends Plugin,
T2 extends Plugin[]
Class extends ClassWithPlugins,
Plugins extends [Plugin, ...Plugin[]],
Copy link
Collaborator

Choose a reason for hiding this comment

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

New little nifty improvement recently added to tuples in TypeScript ✨ no more multiple parameters!

>(
this: S,
plugin1: T1,
...additionalPlugins: T2
): S & {
plugins: any[];
} & Constructor<UnionToIntersection<ReturnTypeOf<T1> & ReturnTypeOf<T2>>>;
this: Class,
...plugins: Plugins,
): Class & {
plugins: [...Class['plugins'], ...Plugins];
} & Constructor<UnionToIntersection<ReturnTypeOf<Plugins>>>;

/**
* Set defaults for the constructor
*
* ```js
* const MyBase = Base.defaults({ version: '1.0.0', otherDefault: 'value' });
* const base = new MyBase({ option: 'value' }); // `version` option is not required
* base.options // typed as `{ version: string, otherDefault: string, option: string }`
* ```
* @remarks
* Ideally, we would want to make this infinitely recursive: allowing any number of
* .defaults({ ... }).defaults({ ... }).defaults({ ... }).defaults({ ... })...
* However, we don't see a clean way in today's TypeScript syntax to do so.
* We instead artificially limit accurate type inference to just three levels,
* since real users are not likely to go past that.
* @see https://github.com/gr2m/javascript-plugin-architecture-with-typescript-definitions/pull/57
gr2m marked this conversation as resolved.
Show resolved Hide resolved
*/
static defaults<
TDefaults extends Base.Options,
S extends Constructor<Base<TDefaults>>
PredefinedOptionsOne,
ClassOne extends Constructor<Base<Base.Options & PredefinedOptionsOne>> & ClassWithPlugins
>(
this: S,
defaults: TDefaults
): {
new (...args: any[]): {
options: TDefaults;
};
} & S;
constructor(options?: TOptions);
this: ClassOne,
defaults: PredefinedOptionsOne
): ConstructorRequiringVersion<ClassOne, PredefinedOptionsOne> & {
defaults<ClassTwo, PredefinedOptionsTwo>(
this: ClassTwo,
defaults: PredefinedOptionsTwo
): ConstructorRequiringVersion<
ClassOne & ClassTwo,
PredefinedOptionsOne & PredefinedOptionsTwo
> & {
defaults<ClassThree, PredefinedOptionsThree>(
this: ClassThree,
defaults: PredefinedOptionsThree
): ConstructorRequiringVersion<
ClassOne & ClassTwo & ClassThree,
PredefinedOptionsOne & PredefinedOptionsTwo & PredefinedOptionsThree
> & ClassOne & ClassTwo & ClassThree;
} & ClassOne & ClassTwo;
} & ClassOne;

static defaultOptions: {};

/**
* options passed to the constructor as constructor defaults
*/
options: TOptions;

constructor(options: TOptions);
}
export {};
5 changes: 5 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,18 @@ export class Base {
);
};
}

static defaults(defaults) {
return class extends this {
constructor(...args) {
super(Object.assign({}, defaults, args[0] || {}));
}

static defaultOptions = { ...defaults, ...this.defaultOptions };
};
}

static defaultOptions = {};

static plugins = [];
}
198 changes: 186 additions & 12 deletions index.test-d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,30 +6,125 @@ import { barPlugin } from "./plugins/bar/index.js";
import { voidPlugin } from "./plugins/void/index.js";
import { withOptionsPlugin } from "./plugins/with-options";

const base = new Base();
const base = new Base({
version: "1.2.3",
});

// @ts-expect-error unknown properties cannot be used, see #31
base.unknown;

const FooBase = Base.plugin(fooPlugin).defaults({
default: "value",
const BaseWithEmptyDefaults = Base.defaults({
// there should be no required options
});

// 'version' is missing and should still be required
// @ts-expect-error
new BaseWithEmptyDefaults()

// 'version' is missing and should still be required
// @ts-expect-error
new BaseWithEmptyDefaults({})

const BaseLevelOne = Base.plugin(fooPlugin).defaults({
defaultOne: "value",
version: "1.2.3",
});

// Because 'version' is already provided, this needs no argument
new BaseLevelOne();
new BaseLevelOne({});

expectType<{
defaultOne: string,
version: string,
}>(BaseLevelOne.defaultOptions);

const baseLevelOne = new BaseLevelOne({
optionOne: "value",
});
const fooBase = new FooBase({
option: "value",

expectType<string>(baseLevelOne.options.defaultOne);
expectType<string>(baseLevelOne.options.optionOne);
expectType<string>(baseLevelOne.options.version);
// @ts-expect-error unknown properties cannot be used, see #31
baseLevelOne.unknown;

const BaseLevelTwo = BaseLevelOne.defaults({
defaultTwo: 0,
});

expectType<string>(fooBase.options.default);
expectType<string>(fooBase.options.option);
expectType<string>(fooBase.foo);
expectType<{
defaultOne: string,
defaultTwo: number,
version: string,
}>({ ...BaseLevelTwo.defaultOptions });
Copy link
Owner Author

Choose a reason for hiding this comment

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

why the destructoring? Isn't this the same as

Suggested change
}>({ ...BaseLevelTwo.defaultOptions });
}>(BaseLevelTwo.defaultOptions);

Copy link
Collaborator

@JoshuaKGoldberg JoshuaKGoldberg Jul 15, 2021

Choose a reason for hiding this comment

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

Ha, this one was nifty. tsd sees an intersection type as not being the same as its equivalent solo type.

  index.test-d.ts:86:0
  ✖  56:0  Parameter type { defaultOne: string; defaultTwo: number; version: string; } is not identical to argument type { defaultOne: string; version: string; } & { defaultTwo: number; }.  


// Because 'version' is already provided, this needs no argument
new BaseLevelTwo();
new BaseLevelTwo({});

// 'version' may be overriden, though it's not necessary
new BaseLevelTwo({
version: 'new version',
});

const baseLevelTwo = new BaseLevelTwo({
optionTwo: true
});

expectType<number>(baseLevelTwo.options.defaultTwo);
expectType<string>(baseLevelTwo.options.defaultOne);
expectType<boolean>(baseLevelTwo.options.optionTwo);
expectType<string>(baseLevelTwo.options.version);
// @ts-expect-error unknown properties cannot be used, see #31
baseLevelTwo.unknown;

const BaseLevelThree = BaseLevelTwo.defaults({
defaultThree: ['a', 'b', 'c'],
});

expectType<{
defaultOne: string,
defaultTwo: number,
defaultThree: string[],
version: string,
}>({ ...BaseLevelThree.defaultOptions });

// Because 'version' is already provided, this needs no argument
new BaseLevelThree();
new BaseLevelThree({});

// Previous settings may be overriden, though it's not necessary
new BaseLevelThree({
optionOne: '',
optionTwo: false,
version: 'new version',
});

const baseLevelThree = new BaseLevelThree({
optionThree: [0, 1, 2]
});

expectType<string>(baseLevelThree.options.defaultOne);
expectType<number>(baseLevelThree.options.defaultTwo);
expectType<string[]>(baseLevelThree.options.defaultThree);
expectType<number[]>(baseLevelThree.options.optionThree);
expectType<string>(baseLevelThree.options.version);
// @ts-expect-error unknown properties cannot be used, see #31
baseLevelThree.unknown;

const BaseWithVoidPlugin = Base.plugin(voidPlugin);
const baseWithVoidPlugin = new BaseWithVoidPlugin();
const baseWithVoidPlugin = new BaseWithVoidPlugin({
version: "1.2.3",
});

// @ts-expect-error unknown properties cannot be used, see #31
baseWithVoidPlugin.unknown;

const BaseWithFooAndBarPlugins = Base.plugin(barPlugin, fooPlugin);
const baseWithFooAndBarPlugins = new BaseWithFooAndBarPlugins();
const baseWithFooAndBarPlugins = new BaseWithFooAndBarPlugins({
version: "1.2.3",
});

expectType<string>(baseWithFooAndBarPlugins.foo);
expectType<string>(baseWithFooAndBarPlugins.bar);
Expand All @@ -42,7 +137,9 @@ const BaseWithVoidAndNonVoidPlugins = Base.plugin(
voidPlugin,
fooPlugin
);
const baseWithVoidAndNonVoidPlugins = new BaseWithVoidAndNonVoidPlugins();
const baseWithVoidAndNonVoidPlugins = new BaseWithVoidAndNonVoidPlugins({
version: "1.2.3",
});

expectType<string>(baseWithVoidAndNonVoidPlugins.foo);
expectType<string>(baseWithVoidAndNonVoidPlugins.bar);
Expand All @@ -51,6 +148,83 @@ expectType<string>(baseWithVoidAndNonVoidPlugins.bar);
baseWithVoidAndNonVoidPlugins.unknown;

const BaseWithOptionsPlugin = Base.plugin(withOptionsPlugin);
const baseWithOptionsPlugin = new BaseWithOptionsPlugin();
const baseWithOptionsPlugin = new BaseWithOptionsPlugin({
version: "1.2.3",
});

expectType<string>(baseWithOptionsPlugin.getFooOption());

JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
// Test depth limits of `.defaults()` chaining
const BaseLevelFour = BaseLevelThree.defaults({ defaultFour: 4 });

expectType<{
version: string;
defaultOne: string;
defaultTwo: number;
defaultThree: string[];
defaultFour: number;
}>({ ...BaseLevelFour.defaultOptions });

const baseLevelFour = new BaseLevelFour();

// See the node on static defaults in index.d.ts for why defaultFour is missing
// .options from .defaults() is only supported until a depth of 4
expectType<{
version: string;
defaultOne: string;
defaultTwo: number;
defaultThree: string[];
}>({ ...baseLevelFour.options });

expectType<{
version: string;
defaultOne: string;
defaultTwo: number;
defaultThree: string[];
defaultFour: number;
// @ts-expect-error - .options from .defaults() is only supported until a depth of 4
}>({ ...baseLevelFour.options });

const BaseWithChainedDefaultsAndPlugins = Base
.defaults({
defaultOne: "value",
})
.plugin(fooPlugin)
.defaults({
defaultTwo: 0,
});

const baseWithChainedDefaultsAndPlugins =
new BaseWithChainedDefaultsAndPlugins({
version: "1.2.3",
});

expectType<string>(baseWithChainedDefaultsAndPlugins.foo);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a tough one! It doesn't repro if either of the .defaults({ ... }) are removed above... Tricky!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it! It was the this: Class generic.

My leading hunch was that whatever information added the foo: string from the .plugin(fooPlugin) call was being abandoned by the .defaults calls going on top of each other. That meant the piece of the type that contained foo wasn't getting preserved.

That piece of type was the this: Class -> & Class. Using just one <Class> generic from the start meant new type information on new this scopes that had extra stuff added to them was being lost.


const BaseWithManyChainedDefaultsAndPlugins = Base.defaults({
defaultOne: "value",
})
.plugin(fooPlugin, barPlugin, voidPlugin)
.defaults({
defaultTwo: 0,
})
.plugin(withOptionsPlugin)
.defaults({
defaultThree: ["a", "b", "c"],
});

expectType<{
defaultOne: string;
defaultTwo: number;
defaultThree: string[];
}>({ ...BaseWithManyChainedDefaultsAndPlugins.defaultOptions });

const baseWithManyChainedDefaultsAndPlugins =
new BaseWithManyChainedDefaultsAndPlugins({
version: "1.2.3",
foo: "bar",
});

expectType<string>(baseWithManyChainedDefaultsAndPlugins.foo);
expectType<string>(baseWithManyChainedDefaultsAndPlugins.bar);
expectType<string>(baseWithManyChainedDefaultsAndPlugins.getFooOption());
Loading