feathersjs-ecosystem / feathers-authentication-management

Adds sign up verification, forgotten password reset, and other capabilities to local feathers-authentication
https://feathers-a-m.netlify.app/
MIT License
246 stars 98 forks source link

[dove] Migration to latest feathers dove version #196

Closed mdartic closed 1 year ago

mdartic commented 1 year ago

Summary

This PR address the migration of this package to work in feathers dove.

This is related to #195 too, as the actual package in 4.0.2 is not 100% compatible with dove.

Other Information

This is still a work in progress. Compilation is ok, and the dist/ is working on my test project. But all tests are not OK. I'm working on it. I may refer to this PR to ask for help.

netlify[bot] commented 1 year ago

Deploy Preview for feathers-a-m ready!

Name Link
Latest commit e45cb386fc933c9ad543f83c24dc1d4fcd3954cc
Latest deploy log https://app.netlify.com/sites/feathers-a-m/deploys/63408e491a11b70009b0c3d5
Deploy Preview https://deploy-preview-196--feathers-a-m.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

fratzinger commented 1 year ago

I also opened a new branch: https://github.com/feathersjs-ecosystem/feathers-authentication-management/tree/dove Could you please make the PR against this branch instead of master? Thanks!

fratzinger commented 1 year ago

feathers v5 is not compatible with node12 anymore. So fam does not need to support it as well. So please remove it here: https://github.com/feathersjs-ecosystem/feathers-authentication-management/blob/master/.github/workflows/node.js.yml#L13 And here: https://github.com/feathersjs-ecosystem/feathers-authentication-management/blob/master/package.json#L29

The minimal supported node version is >= 14 as can be seen here: https://github.com/feathersjs/feathers/blob/dove/package.json#L23

That's why the CI test for 12.x fails.

mdartic commented 1 year ago

Tests are locally ok after patching @feathersjs/error package as mentioned here. Waiting for a fix on this package, and tests will work ok after updating dependencies. I'm still facing issues with feathers-hooks-common package and iff / iff.else combined with addVerification / removeVerification hooks. I'm trying to understand why.

mdartic commented 1 year ago

Issues fam+fhc are now understood. They were on my side. Waiting for error feedback. @fratzinger do you see anything else ? And do you agree with my thoughts on error package ?

fratzinger commented 1 year ago

Without any further looking. What about: const err = new BadRequest(undefined, data) ?

mdartic commented 1 year ago

This "could" work as BadRequest constructor signature accept it :

export class BadRequest extends FeathersError {
  constructor(message?: ErrorMessage, data?: any) {
    super(message, 'BadRequest', 400, 'bad-request', data)
  }
}

But, as FeathersError doesn't accept it (constructor(err: ErrorMessage, name: string, code: number, className: string, _data: any)), I think either BadRequest shouldn't accept a message?: ErrorMessage or FeathersError should accept a err?: ErrorMessage. This seems more consistent ?

Actually, my IDE is not happy with error file for this reason. image

Do you prefer I use your hack ?

mdartic commented 1 year ago

Dove has been upgraded to pre.31 with a fix on error package. Tests are now ok. I think this PR is now ready to review !

fratzinger commented 1 year ago

Thanks for your work! Released as feathers-authentication-management@5.0.0-pre.0 🎉

mdartic commented 1 year ago

Thanks !