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

[5.0.0-beta.2] Migrating Hooks And Services Generated By Feathers CLI #2307

Closed
forgot opened this issue Apr 20, 2021 · 6 comments · Fixed by #2310
Closed

[5.0.0-beta.2] Migrating Hooks And Services Generated By Feathers CLI #2307

forgot opened this issue Apr 20, 2021 · 6 comments · Fixed by #2310

Comments

@forgot
Copy link

forgot commented Apr 20, 2021

I'm attempting to migrate from v4.5.9 to v5.0.0-pre.2, and have run into some complaints from the Typescript compiler. I've read through the migration guide, as well as all the Dove version of the documentation, but I'm having some difficulty resolving the issues.

Hooks

Currently, a hook generated by the CLI looks like this:

import { Hook, HookContext } from '@feathersjs/feathers';

export default (options = {}): Hook => {
  return async (context: HookContext): Promise<HookContext> => {
    return context;
  };
};

Importing Hook throws the following: Module '"@feathersjs/feathers"' has no exported member 'Hook'. What changes need to be made here? Is this simply a definitions issue, or should the hook be created a different way?

Additionally, several hooks throw the following when used inside an iif hook from feathers-hooks-common:

Argument of type '(context: HookContext<any, any>) => Promise<HookContext<any, any>>' is not assignable to parameter of type 'Hook<any, Service<any>>'.
  Types of parameters 'context' and 'context' are incompatible.
    Type 'HookContext<any, Service<any>>' is missing the following properties from type 'HookContext<any, any>': arguments, self

Services

Services generated by the CLI create three files: *.class.ts, *.hooks.ts, and *.service.ts. With a fresh service from the CLI, two errors are thrown.

Here we get the error Generic type 'ServiceAddons<A, S>' requires 2 type argument(s).

declare module '../../declarations' {
  interface ServiceTypes {
    'test': Test & ServiceAddons<any>;
  }
}

From reading through some of the issues, it looks like this can be changed to the following without issues. Is this correct?

declare module '../../declarations' {
  interface ServiceTypes {
    'test': Test
  }
}

The second issue appears on this line:
app.use('/test', new Test(options, app));

Here the error reads

No overload matches this call.
  The last overload gave the following error.
    Argument of type 'string' is not assignable to parameter of type 'RequestHandler<ParamsDictionary, any, any, ParsedQs, Record<string, any>>'.

I noticed that changing this line to app.use('test', new Test(options, app)); clears the error, but I have no idea if this is the correct fix.


I know this is all still beta, but I wanted to jump in early as the update to Grant 5 will allow me to do some things that I haven't been able to. I appreciate any insight as to how and what to change to clear the errors and allow the project to compile successfully.

@forgot forgot changed the title Migrating Hooks And Services Generated By Feathers CLI [5.0.0-beta.2] Migrating Hooks And Services Generated By Feathers CLI Apr 20, 2021
@daffl
Copy link
Member

daffl commented Apr 21, 2021

This should be fixed now in v5.0.0-pre.3 - Hook is exported as LegacyHookFunction

@forgot
Copy link
Author

forgot commented Apr 21, 2021

@daffl If they are now exported as LegacyHookFunction, is there a more preferred way to create hooks?

Additionally, what about the errors thrown when using hooks from feathers-hooks-common? I know that's a different repo, and I can open another issue there, but these are the of errors I'm getting when calling hooks within an iff hook...

Calling iff(true, authenticate('jwt')) throws:

Argument of type '(context: HookContext<any, any>) => Promise<HookContext<any, any>>' is not assignable to parameter of type 'Hook<any, Service<any>>'.
  Types of parameters 'context' and 'context' are incompatible.
    Property 'arguments' is missing in type 'HookContext<any, Service<any>>' but required in type 'HookContext<any, any>'.

Calling .else( hashPassword('password') ) throws:

Argument of type '(context: HookContext<any, any>) => Promise<HookContext<any, any>>' is not assignable to parameter of type 'Hook<any, Service<any>>'.

Calling iff(someHookThatReturnsBoolean(), anotherHook()) throws:

Argument of type '(context: HookContext) => Promise<boolean>' is not assignable to parameter of type 'boolean | PredicateFn'.
  Type '(context: HookContext) => Promise<boolean>' is not assignable to type 'SyncPredicateFn'.
    Types of parameters 'context' and 'context' are incompatible.
      Type 'HookContext<any, Service<any>>' is not assignable to type 'HookContext<Application<any, any>, any>'.

I'm assuming this is because the feathers-hooks-common repo doesn't have the new definitions, correct?

@daffl
Copy link
Member

daffl commented Apr 21, 2021

Correct. Probably similar issues for the database adapters. You could open an issue in the related repo but a pull request would be even more welcome.

@forgot
Copy link
Author

forgot commented Apr 21, 2021

If the definitions are for Dove, I'm assuming any pull requests should have a new branch with the changes applied there?

@forgot
Copy link
Author

forgot commented Apr 28, 2021

@daffl I've created pull requests for both feathers-hooks-common and feathers-sequelize that allow them to work with 5.0.0-pre.3. Neither repo has a Dove branch, so the pull request are comparing to Master. I am successfully able to use both packages with 5.0.0-pre.3 using the forks.

@daffl
Copy link
Member

daffl commented Apr 28, 2021

Awesome, thank you for doing that! I'll have a look at how to roll those out (probably as a pre-release for now, too) - looking at some small database adapter API improvements (mostly #2103) for v5 after that as well.

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 a pull request may close this issue.

2 participants