Skip to content

Conversation

fratzinger
Copy link
Collaborator

@fratzinger fratzinger commented Jan 21, 2021

This is a port to typescript, as it was discussed here before. It seems that the solution from @MohammedFaragallah is not actively maintained. So hopefully we can work out a typescript-version, which gets merged.

Todos:

  • Port src to typescript is pretty much done with types
    • Typescript
    • propper typings
    • service class
    • make sure client is backward compatible
  • Port test to typescript
    • Typescript
    • propper typings
  • seperate services/routes as suggested in Rework #144
    • seperate Class with create
    • call services create methods in main service for backward compatibility
    • one service for every action
    • use _create for every service to make it possible to skip hooks as for adapter-commons
    • make sure service.publish does not get hit to prevent channel emit
    • register services under route (app.use)
    • add more tests for this
  • add docs
    • Vuepress

New Stuff:

  • added option: passwordField for dynamic passwordField. It was a fix variable before
  • added services
  • can call _create function on all services to bypass hooks
  • separate services for each action (except action: 'options')

IDE-Support:

image

- complete typing
- update dependencies
- semistandard -> eslint-typescript
- start converting tests to typescript
- ref #144
- identityChange had no 'field' property
- tests fixes
- add passwordField to options
- propper typing
- call seperate services from main service
- destructure create-data for better IDE support
- move AuthenticationManagementService to seperate file
@daffl
Copy link
Member

daffl commented Jan 26, 2021

Thank you for putting this up! I sent you an invite to the repository so you can work with it more directly. I'll be happy to give some pointers on repository organization but will leave review up to the other maintainers. One thing to maybe consider since it is already a refactoring would be how the API could be improved working with the upcoming more custom methods feathersjs/feathers#1976

@fratzinger
Copy link
Collaborator Author

Thanks for your trust. I happily accepted the invite.
I will keep this PR here for some time until the other maintainers are totally fine with it.
My plan would be to release a pre-version first. If someone got that working, maybe we can work out a way to v4. As of now, I think it would be the best to integrate some breaking changes. But I want to discuss it with the other maintainers first.

I will speak up, when I got something new.

- completely move `passwordField` to options
@TheSinding
Copy link
Collaborator

This is awesome, thanks for the work !

@fratzinger
Copy link
Collaborator Author

Thanks :) I'm not quite through though. Currently I have not that much time, but I'm willing to get this finished.
If you had something to add for now, I would be happy. Otherwise I will come to this again in a few weeks 😃

- Base class with publish-prevention
- `useSeparateServicePaths` option
-  use internal service.options instead of create({ action: 'options' })
- add tests for publish
- add tests for `useSeparateServicePaths`
- more typings
@fratzinger
Copy link
Collaborator Author

I think this is getting somewhere and I want to open a discussion on some thoughts, that I have:

  1. opt-in or opt-out separate services by default? As of now it's enabled by default Rework #144
  2. paths for separate services as of now are quite long (see below). Are there suggestions for better/shorter names?:
{ // `path` is the provided authManagement-path ('authManagement' by default)
    checkUnique: `${path}/check-unique`,
    identityChange: `${path}/identity-change`,
    passwordChange: `${path}/password-change`,
    resendVerifySignup: `${path}/resend-verify-signup`,
    resetPwdLong: `${path}/reset-password-long`,
    resetPwdShort: `${path}/reset-password-short`,
    sendResetPwd: `${path}/send-reset-pwd`,
    verifySignupLong: `${path}/verify-signup-long`,
    verifySignupSetPasswordLong: `${path}/verify-signup-set-password-long`,
    verifySignupSetPasswordShort: `${path}/verify-signup-set-password-short`,
    verifySignupShort: `${path}/verify-signup-short`
}

@claustres @TheSinding What do you think about the two points above? And what do you think about this PR at all? Any pointers, criticism, etc. is highly appreciated! Are there points, that disturbed you in feathers-authentication-management, which should be changed in an upcoming major release?
My next step is to improve the vuepress-docs.

Any comments are appreciated :)

@OnnoGabriel
Copy link
Contributor

Hi @fratzinger, this is huge, thanks a lot! Just to clarify what you did: this is not only a port to TypeScript, but you are also going to implement the "REST wrapper" discussed in #144, right? Is this what you mean with "separate services"? And will this new version of the package be fully backward compatible?

@fratzinger
Copy link
Collaborator Author

  1. "REST wrapper": yep, that's what I meant.
  2. fully backward compatible: I'm on it. There could be some quirks. One that I know of, is the client part. I have to work on that one (less Typescript magic, I guess). But yes, my intention is, to make it fully backward compatible. Nonetheless I would make a major release though. At least I think of marking some things as DEPRECATED (the actions with the REST wrapper as a successor) and then remove them anytime in the future.

- export defaultOptions -> addVerification - path
- rearrange useSeparateServices
- divide Service & configure functions
- ensure options per Service
- types: better types
- chore: update dependencies
- tests: add tests for separate services
- tests: increase about-time to 600
- better typing (abstract _create with type)
- sort actions alphabetically
- overview page
- started services page
@fratzinger
Copy link
Collaborator Author

closed in favor of #164 . See ongoing work over there.

@fratzinger fratzinger closed this Apr 18, 2021
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.

4 participants