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

Unknown keys on instance should not be typed as any when instantiating from Base.defaults() or Base.plugin() #31

Closed
gr2m opened this issue Apr 1, 2021 · 7 comments
Labels

Comments

@gr2m
Copy link
Owner

gr2m commented Apr 1, 2021

Example

const base = new Base()

// TS complains as expected
base.unknown

const BaseWithDefaults = Base.defaults({})
const baseWithDefaults = new BaseWithDefaults

// TS does _not_ complain, `.unknown` is typed as `any`, and it shouldn't
baseWithDefaults.unknown

The problems are these two lines from the resulting .d.ts file

I don't quite understand whey they are coming from, and how I can change them to at least [x: string]: unknown.

Any idea?

Here is a playground with the full source code

@Andarist
Copy link

Andarist commented Apr 2, 2021

Can you drop the generic contraint S extends Constructor<any>? I don't quite see why you need it there - you shouldn't be able to parameterize it but rather the base class should flows/compose through layers of .defaults() calls etc. Or am I missing something?

Without it works:
TS playground (with some extra casting in the static plugin method)

@gr2m
Copy link
Owner Author

gr2m commented Apr 2, 2021

Thanks a lot for looking into it!

Can you drop the generic contraint S extends Constructor<any>?

It came from here:
https://stackoverflow.com/questions/58636914/chaining-static-plugin-class-method-to-extend-instance-apis-multiple-times/58706699#58706699

With your changes, Base.plugin(myPlugin).defaults(options) no longer works. This test is now failing

it(".plugin().defaults()", () => {
const BaseWithPluginAndDefaults = Base.plugin(fooPlugin).defaults({
baz: "daz",
});
const BaseWithDefaultsAndPlugin = Base.defaults({
baz: "daz",
}).plugin(fooPlugin);
const instance1 = new BaseWithPluginAndDefaults();
const instance2 = new BaseWithDefaultsAndPlugin();
expect(instance1.foo()).toEqual("foo");
expect(instance1.options).toStrictEqual({ baz: "daz" });
expect(instance2.foo()).toEqual("foo");
expect(instance2.options).toStrictEqual({ baz: "daz" });
});

TS Playground to showcase the problem

@Andarist
Copy link

Andarist commented Apr 2, 2021

Oh, I thought that TS would be smart enough to understand that this type should infer from creates subclasses 😬

Casting return values with typeof this somewhat works but I guess this might also be incorrect for some other reason:
TS playground

@gr2m
Copy link
Owner Author

gr2m commented Apr 3, 2021

Thanks, that works with my tests so far. I'll ship it in @octokit, we'll see how it goes!

@gr2m gr2m closed this as completed Apr 3, 2021
gr2m added a commit to octokit/core.js that referenced this issue Apr 3, 2021
@Andarist
Copy link

Andarist commented Apr 5, 2021

Ping me if you encounter any problems - quite frankly this API was somewhat hard to grok at first for me and I was unsure what are exact constraints that you want to achieve so it's hard for me to assess if this will truly "just work" for everything you want to achieve.

I'm not entirely sure why this was the problem but I think this was because you were using mixin types/feature (I was not aware that TS supported this~ up to this point) and that using those triggered some different code path in TS that resulted in this. My "fix" just works around this by not using those mixin semantics and just implementing "mixin" by hand at the type-level.

@github-actions
Copy link

github-actions bot commented Apr 5, 2021

🎉 This issue has been resolved in version 2.0.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

No branches or pull requests

3 participants
@gr2m @Andarist and others