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

Use stricter type for HookContext 'method' prop #1896

Merged
merged 1 commit into from
Apr 5, 2020

Conversation

JorgenVatle
Copy link
Contributor

A simple tweak to the HookContext's method property type declaration. This specifies a union type with the possible string types for the method property. Essentially allowing for autocompletes by the IDEs that support TypeScript, as well as warnings should you mistype a method name.

@daffl daffl merged commit 24a41b7 into feathersjs:master Apr 5, 2020
@bertho-zero
Copy link
Collaborator

What about custom methods?

@JorgenVatle
Copy link
Contributor Author

JorgenVatle commented Apr 6, 2020

This only affects the TypeScript type definitions for a hook's context.method property. Actual custom service methods should remain unaffected. 👍

I'm unsure whether context.method is something that can have another value than the defaults provided by Feathers. If this is not the case, and you do have custom method types; you'd be able to override the type definition for your project within a global.d.ts file.

@bertho-zero
Copy link
Collaborator

I added support for custom methods with this PR, it is something common and that I do very often.

I do not use TypeScript and I am embarrassed to have to overload the type definition in all my projects.

@JorgenVatle
Copy link
Contributor Author

JorgenVatle commented Apr 6, 2020

Oh, very cool. I was not aware of that. 👀

This should only affect TypeScript projects, so assuming you're not using TypeScript, no action should be necessary. 👍 If you are using TypeScript, you'll only get a compile warning. Your project should still compile and run as before.

@JorgenVatle
Copy link
Contributor Author

JorgenVatle commented Apr 6, 2020

I'll look into automatically incorporating custom method names into the type definition here. 👍

@bertho-zero
Copy link
Collaborator

It does not affect me directly. My IDE, IntelliJ IDEA, will use these types and maybe show me warnings, we'll see.

@JorgenVatle
Copy link
Contributor Author

JorgenVatle commented Apr 6, 2020

Had a quick look for you, PhpStorm 2020.1 apparently does not trigger any warnings for comparisons using custom method names when working with a .js file.
image
I think this could differ depending on your linting setup though. If you do see an error, you should be able to suppress it.

We could rather easily address the issue by appending string to the union type. This would certainly prevent any warnings, but that is also part of the problem. You'd want to see a warning in case you end up typing out a method name that doesn't exist.

@JorgenVatle
Copy link
Contributor Author

I had a closer look at the type definitions for services; the current type definitions have limited support for custom methods. Opting for the any type on the ServiceMethods interface.

It's likely going to end up taking a pretty serious refactor to Feathers' service type definitions to properly support inferred custom service methods. While it'd be a pretty good productivity boost for most TypeScript users, this is unfortunately not something I would be able to take responsibility for at the moment.

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

Successfully merging this pull request may close these issues.

None yet

3 participants