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

Add .decorator() method #61

Merged
merged 29 commits into from
Mar 18, 2021
Merged

Conversation

Richienb
Copy link
Contributor

@Richienb Richienb commented Aug 20, 2020

Signed-off-by: Richie Bendall <richiebendall@gmail.com>
@sindresorhus
Copy link
Owner

It should be clearly documented that it's only for TypeScript consumers.

I'm also not sure about the "mixin" naming. "mixin" is a pretty overloaded term and it doesn't exactly fit here.

@Richienb
Copy link
Contributor Author

Richienb commented Aug 21, 2020

@sindresorhus > I'm also not sure about the "mixin" naming.

See how it's done in emittery. What should we call it instead?

test.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
index.d.ts Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

See how it's done in emittery.

It's called that there because the decorator actually "mixes in". That's not about the decorator itself.

What should we call it instead?

Any suggestions?

Signed-off-by: Richie Bendall <richiebendall@gmail.com>
@Richienb
Copy link
Contributor Author

@sindresorhus .decorator()?

index.d.ts Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

@sindresorhus .decorator()?

Yeah, I cannot think of something better than that.

Signed-off-by: Richie Bendall <richiebendall@gmail.com>
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
@Richienb Richienb changed the title Add .mixin() Add .decorator() Aug 22, 2020
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
index.d.ts Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title Add .decorator() Add .decorator() method Aug 22, 2020
test.js Outdated Show resolved Hide resolved
readme.md Show resolved Hide resolved
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
Richienb and others added 4 commits October 11, 2020 11:39
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
@Richienb
Copy link
Contributor Author

Where did the build statuses go?

@sindresorhus
Copy link
Owner

b1c65cd

@Richienb
Copy link
Contributor Author

@sindresorhus Are you at the point of switching to Github Actions for all of your projects?

Base automatically changed from master to main January 23, 2021 17:35
@sindresorhus
Copy link
Owner

@Richienb Still interested in finishing this? I noticed it's still marked as a draft PR and tests are failing.

@Richienb
Copy link
Contributor Author

Richienb commented Mar 7, 2021

Yes - most the errors look to be TypeScript compilation errors so I'll get to fixing them

Signed-off-by: Richie Bendall <richiebendall@gmail.com>
>(
options: Options<FunctionToMemoize, CacheKeyType> = {}
) => (
target: any,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think there's a way to avoid this being any.

Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't it be:

Suggested change
target: any,
target: FunctionToMemoize,

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

target refers to the object that the function lives in. const input: FunctionToMemoize = target[propertyKey].

Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't it at least be object then to ensure undefined and null are not the target? Also, I generally prefer unknown over any whenever possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

object isn't allowed by xo which wants it to be Record<string, unknown> which doesn't work either because a class is not an plain object.

Copy link
Owner

Choose a reason for hiding this comment

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

Not using object is just a general rule. You shouldn't use any either, ideally. But sometimes we need exceptions. Just use an eslint-ignore comment.

Copy link
Contributor Author

@Richienb Richienb Mar 18, 2021

Choose a reason for hiding this comment

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

As soon as I change any to object, the compiler throws because {} can't be indexed by a string.

If I try to use generics with {target: Target, propertyKey: keyof Target}, the compiler (possible due to a bug) is instead unable to resolve a target properly so it always falls back to unknown, making propertyKey resolve to never.

It looks like setting it to anything else, is nearly impossible.

@Richienb Richienb marked this pull request as ready for review March 9, 2021 11:07
index.ts Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
readme.md Outdated Show resolved Hide resolved
>(
options: Options<FunctionToMemoize, CacheKeyType> = {}
) => (
target: any,
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't it be:

Suggested change
target: any,
target: FunctionToMemoize,

?

index.ts Outdated Show resolved Hide resolved
index.ts Outdated Show resolved Hide resolved
Richienb and others added 4 commits March 10, 2021 23:00
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
Signed-off-by: Richie Bendall <richiebendall@gmail.com>
@sindresorhus
Copy link
Owner

#61 (comment)

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

Successfully merging this pull request may close these issues.

Add function decorator
3 participants