V5.0 dev

[nkitku]

Rewrite of whole library in typescript

[thomseddon]

@nkitku thank you for this truly heroic effort, this is a very impressive and thorough start, this certainly looks like a brilliant direction for the project to take.

This needs a thorough review to check backwards compatibility and RFC compliance.

As a quick start on that road, could you resolve the conflicts shown in the PR so we can start working from a mergeable branch? Additionally, I notice that the files are now all using 0755 permissions, I think best practice is 0644 for files and 0755 for directories?

[nkitku]

thank you for appreciation,
ok, i'll try to resolve all conflicts
and thanks for suggestion, i'll fix all file permissions

[nkitku]

@thomseddon instead of merging it in 'dev' branch can we have new branch "v5.0-dev"?

[nkitku]

@thomseddon i have resolved all conflicts

[peter-fan-cn]

$ npm test

[email protected] pretest /mnt/d/codes/node-oauth2-server
npx jshint --config ./.jshintrc lib test

npx: installed 30 in 30.987s
ERROR: Can't find config file: ./.jshintrc
npm ERR! Test failed. See above for more details.

[peter-fan-cn]

@nkitku
I think there must be a copy for users who are not using typescript.
so, how about add the build result to the repo.

[nkitku]

@peter-fan-cn
i think you clone master branch please use branch 'v5.0-dev', and thank you for reminding me i'll fix master branch later,

and for development process we must require typescript but for publishing it to npm we will use build version

npm run build:prod

[peter-fan-cn]

@peter-fan-cn
i think you clone master branch please use branch 'v5.0-dev', and thank you for reminding me i'll fix master branch later,

and for development process we must require typescript but for publishing it to npm we will use build version

npm run build:prod

Yes, you are right. thank you for your reply.

[emilcardell]

Hi,
The typescript upgrade looks like it is done. Is there a possibility to make npm release 5.0.0-dev.1 to make it easier to create and contribute examples and documentation?

[liyamahendra]

@thomseddon i am interested to contribute in this project. Are you still looking for contributors / maintainers?

[thomseddon]

@liyamahendra yes!

There are three main areas we need help on:

• Review this branch - this is a phenomenal start, but we could do with another set of eyes on it. Assistance in reviewing the code and implementation would be brilliant
• Documentation - we need to document usage of this new branch and also create an upgrade guide for users
• Examples

Help on the above would be hugely appreciated. I will add anyone who makes meaninful contributions as a permanent maintainer :)

[thomseddon]

@emilcardell I wouldn't normally create an official npm release until we have something stable, this branch will remain a development branch until we've had some review (I'm also asking for help on this). Testing and examples can be done by referencing the repo directly in your package.json

[benaubin]

Noticed that you switched from ESLint to TSLint - TSLint is going to be deprecated in favor of ESLint by the end of the year, so it may be better to stick with ESLint.

[sessionboy]

Will v5 support graphql? I expect so...

[ankon]

@nkitku I'm actually fixing most of the things pointed out above in our own fork of your branch, so I think instead of just pointing out problems I could also create PRs against your v5.0-dev branch for these issues. WDYT?

[nkitku]

@nkitku I'm actually fixing most of the things pointed out above in our own fork of your branch, so I think instead of just pointing out problems I could also create PRs against your v5.0-dev branch for these issues. WDYT?

@ankon you are welcome
But I think all the amendments in the library should go from here so that the community can discuss,
Others such as improvements in rewriting, testing and document updates can be

[thomseddon]

@ankon thank you for your great review work so far, hugely appreciated!

@nkitku I'm thinking the plan to move forward witht this branch could be for me to make you a collaborator, and for us to merge this into a new 5.x branch, we can continue to take PRs/Issues against that branch. We can have another PR open tracking a todo list for release; docs, tests, spec compliance - let me know if there's any more review you want to pick up before I merge this PR into the new branch

[ankon]

This is scary: Two requests in parallel could produce quite some confusion here it seems. The model is shared between all handlers (and whatever else gets it through the options), and only passed by reference.

(This is a problem also in the original code)

[Uzlopak]

We should remove the request from the model and see if something breaks. I think it is only bad if you really access it.

[nkitku]

@nkitku I'm thinking the plan to move forward witht this branch could be for me to make you a collaborator, and for us to merge this into a new 5.x branch, we can continue to take PRs/Issues against that branch. We can have another PR open tracking a todo list for release; docs, tests, spec compliance - let me know if there's any more review you want to pick up before I merge this PR into the new branch

@thomseddon thank you, i think so too, so far so good

[tvvignesh]

@nkitku @thomseddon I can help you guys out by testing as an end user and giving you the feedback on the same. Let me know when, and I will do it. Thanks.

[14gasher]

@ankon You are a boss.

[Uzlopak]

Is there any good fork of this, which got already publicated on npm?

[ankon]

Mostly for the record: After trying to get this running for a few days we re-evaluated our actual goals, and found that a OAuth server would not work in our environment. For a bit of context: https://medium.com/collaborne-engineering/replace-cognito-hosted-ui-d7619d037036

[karocksjoelee]

Hi, do we have an estimate release date on v 5.0 ? Or how can I help ?

[mayrbenjamin92]

Hi guys - the ongoing discussion has been started last year 😄 so when will this branch get merged?

[alexanderkjeldaas]

@ankon seems like most of your issues have been resolved, but did you publish your branch anywhere?

[thomseddon]

Thank you for the comprehensive review @ankon - if we could get these and all other pending items resolved, then I'd be happy to cut a public beta for feedback

[Uzlopak]

@thomseddon

Beside this: Do you want to add code coverage metrics to this branch too?

[Uzlopak]

I mean:

I could add the necessary changes ;)

[alexanderkjeldaas]

I added some fixes in nkitku#4

A few things that are unclear to me that I 'fixed' are:

• I think Client and User should be required in the models and the APIs.
• Because fo the above, I changed some tests that invoked functions without arguments to be "explicitly sloppy" by doing things like undefined as unknown as Client etc.
• scope should be optional in the model functions.
• it's unclear whether an undefined scope should call model.verifyScope or not.

[Uzlopak]

Added code coverage and removed the request from the model

nkitku#5

[ankon]

@ankon seems like most of your issues have been resolved, but did you publish your branch anywhere?

For the time where I tried to get this working I added commits here: https://github.com/Collaborne/node-oauth2-server/commits/collaborne/v5.0-dev

But, as mentioned before, we actually decided to abort this direction for us, and instead focus on a different approach completely. As such: None of these commits there are intended for merging anywhere, although of course you can have a look and pick them into another fork if you want to.

[Uzlopak]

How about a release? Or atleast a merge into the dev-branche, so that we can make pull requests into the oauthjs dev branch?

[thomseddon]

Merged into the new v5-dev branch, we can use that for all PRs going forward! 🎉

There's still some outstanding review from this PR, e.g. regarding a few tests, if you'd be able to pick up @nkitku? I have made you a contributor, I will update the Readme now, pointing to the new branch, I'll go back and label/close a lot of the old issues.

[nkitku]

@thomseddon thanks, i will try to review all tests again.

[jankapunkt]

Is there any significant issue preventing this from being merged?

[Uzlopak]

Fyi, these are the plans for this project

#621