Rework

[TheSinding]

I was thinking about whether we should rework the repo.
I have two suggestion in mind.

1. Moving the source code to a TypeScript base.
   I would argue this is more maintainable and this would also help with intellisense in various IDE's.

2. This is more radical - Moving away from actions in data and instead use authmanagement/{action} .
   So a reset password would move from

{
    "action": "resetShortToken",
    "data": { ... }
}

To a more REST like approach with /authmanagement/reset
This would make it work more like a REST API and as well as a standard feathers service.

I realize this would be breaking changes, but I think it would benefit the overall structure of calling the API

Let me know what you think, and if I should elaborate.

[claustres]

Personally I am not convinced at all by TypeScript, I share a lot with https://medium.com/javascript-scene/the-typescript-tax-132ff4cb175b notably that type safety doesn’t seem to make a big difference and 99% of the bugs are not linked to it (I come from a long C++ background and I have a good idea about this). Moreover, I was using babel for a while in order to create isomorphic code and now Nodejs is supporting most of ES6 including import/export I am pretty happy to go away from transpiling. As a consequence TS seems to me a step back. However, this is a community-driven project and if most people prefer to go for TS I don't have any problem with this, I will try to help.

About the more "REST" approach I would argue that it will completely break the API and this is not really what users want except if it is balanced by huge improvements in functionalities, security or whatever. Moreover, a lot of people are using it with websockets and are far less concerned by this topic. Last but not least, it seems to be that the current implementation relies on the "recommended way" by the Feathers team itself (see eg feathersjs/feathers#488 and https://docs.feathersjs.com/help/faq.html#what-node-versions-does-feathers-support). But maybe you can detail a little bit more your though, do you want to create a service with a create method per authentication management operation for instance ?

Thanks for taking time to try enhancing the module, I hope others will provide their own views.

[MohammedFaragallah]

I'm working on a typescript fork of this module and will be ready in a few days for me the IntelliSense alone is enough many actions with different properties, if you decided to migrate I'll be very happy to participate.

[claustres]

PRs are welcome, waiting for your feedback.

[MohammedFaragallah]

it will take some work to merge the changes here so please give it a look and if everything is good I'll open a PR.

https://github.com/MohammedFaragallah/feathers-authentication-management-ts
https://www.npmjs.com/package/feathers-authentication-management-ts

I tested it in JS/TS projects there are no breaking changes only some type related changes for TS projects

- "feathers-authentication-management": "^2.0.1",
+ "feathers-authentication-management-ts": "^1.0.0",

[claustres]

I think a PR is the good way to go because we can then easily compare changes and review code. Do you plan to migrate the tests as well ? That's probably the best way to see if there are no breaking changes.

Not sure to have time to look at it really soon however.

Thanks again.

[OnnoGabriel]

I would test the TS version in our app, however, it seems it has to be installed with Yarn, which we do not use in our projects:

feathers-authentication-management-ts@1.0.0 preinstall: `node -e "if(process.env.npm_execpath.indexOf('yarn') === -1) throw new Error('feathers-authentication-management-ts must be installed with Yarn: https://yarnpkg.com/')"`

In general, I agree with claustres. There are benefits in using TypeScript, but in our Feathers apps we still use pure NodeJS/ES6. And we hesitate to migrate just for type safety and intellisense. But I will not stand against it, if this package is moving to TS.

[claustres]

We have to be a little careful about the maintenance of this project. We are just trying to take it over, so if new maintainers are coming, willing to support and think it will be better with TS go for it. However, if this is just a one shot to be "up-to-date" and then the current maintainers are forced to manage a new code version and tooling they don't know without any support after the migration I am pretty reluctant at this stage. Let me know what you think.

[TheSinding]

I see both of your points @OnnoGabriel and @claustres and I agree if we are moving to typescript we should have some sort of migration plan.

Regarding the RPC and actions the reasoning I had was regarding the hooks of a service.
We started with the RPC approach in our project, but the x.hooks.{js|ts} file of a service quickly became cluttered with hooks which would only run when a specific action was given.
But as I'm typing this, I realize that this is only a "issue" behind the scenes and wouldn't benefit the user. Though I still argue that it would be a better approach to split out the actions into paths, if this were a purely REST API.
IMO it would better describe the API.

But as you argue, people mostly use sockets, and it would only be a pain for those and I totally agree.
Also it wouldn't be an improvement in performance and only break the API for all users.

[MohammedFaragallah]

it seems it has to be installed with Yarn

fixed

I think a good start would be adding types without typescript for IntelliSense and type safety in projects using TS.

[TheSinding]

@MohammedFaragallah - I agree, that makes total sense.

[bwgjoseph]

If I may chip in.. I like the idea of REST-like approach (e.g authmanagement/reset) and so on as mentioned by @TheSinding but that would meant a number of services rather than a single service, am I right? Using { data } is a little tough on documentation wise, since it's a single service with multiple types. Not sure if can define clearly in docs like swagger.

I have a single (feathers) backend which supports for WS/REST connection for different type of client (web, mobile, apps), hence, having a more structure and defined service(s) is definitely going to make it a lot better for my case. Breaking change for sure, but I do see the benefit of it.

As of whether to move to typescript, I think we should start by providing @types rather than diving straight into refactoring the entire library as typescript-based.

[claustres]

I am wondering if we cannot simply provide a REST wrapper connecting routes to service operations as for sockets it is not so relevant (although we could also provide an event wrapper). No breaking change, nothing intrusive, what do you think ?

[TheSinding]

@bwgjoseph
True, we should provide the @types as it would be a big job to refactor all the code.

@claustres
That's not a bad idea actually and it would be a fairly easy job as well (famous last words)