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

Major Rewrite (typescript, tests, abstract drivers, documentation) #238

Merged
merged 103 commits into from
Sep 20, 2022

Conversation

Outternet
Copy link

@Outternet Outternet commented Sep 9, 2022

I am a professional oss developer and a client of mine asked to rewrite this library. I completed this and handed it over to my client and my client successfully ran it in production. I asked my client if he wanted to support the community and make my work public / MIT licensed, my client agreed. This rewrite adds typescript, tests, documentation an abstract driver system and closes all open issues at the time of hirering. I can't offer long-time support, but this is a full v5.x.x rewrite with documentation. The only thing I could not share is my clients CI setup and code documentation.

As a courtesy, I will go through all the discussions and issues to see to what extent they are still relevant and link the resolved ones. The only thing left to add is a CI configuration, some more code documentation, and meaby a bit more logic splitting.

@Outternet
Copy link
Author

After going through all the issues, an enum with error messages might still be a good option to add. Many issues can be closed as resolved or out-of-scope.

@Outternet
Copy link
Author

Outternet commented Sep 9, 2022

This is the end of my support for today, if you have any questions left do not hesitate to post a message.

@Outternet
Copy link
Author

Outternet commented Sep 10, 2022

While using the library on the customer's system, I came across a few small things that could be nicer, I have synchronized the internal library with the pull request. also, many older versions of typescript are now supported.

@Outternet
Copy link
Author

Outternet commented Sep 14, 2022

I processed and replied to your comments and made some more types sctricter, also I made the package system better to match the types that were differently defined in @types/passport-jwt, maybe there should be something about that in the migration guide and something about older node versions. Do you agree with the node >= 12 support for v5?

About NestJs, I think they got a lot of questions about this library especially when there were fewer answers here. So they probably got a little irritated and are afraid of getting more questions if there is a breaking change, but the explanation I gave there is still a valid one and I really think this is the best way. Of course, I'm open to more discussion on this topic.

Tthanks for the compliment I do my best to learn English as a second language but sometimes you come across words that are very specefic.

@Outternet
Copy link
Author

@mikenicholson you still there?

@mikenicholson
Copy link
Owner

Do you agree with the node >= 12 support for v5

I think this should be OK. We can release under '@next' and see if there are any concerns raised.

I'm going to edit this PR to merge to a new 'develop' branch and merge today. I appreciate the consistent improvement, although it makes review difficult since I need to review new commits each time. Any further changes can be made as PRs to the develop branch, which can be reviewed individually.

@mikenicholson mikenicholson changed the base branch from master to develop September 20, 2022 02:25
@mikenicholson mikenicholson merged commit 1622c1c into mikenicholson:develop Sep 20, 2022
mikenicholson added a commit that referenced this pull request Sep 20, 2022
@Outternet
Copy link
Author

Outternet commented Sep 20, 2022

I fully understand and agree, it had become a bit bigger than I had originally planned. On another note were you able to look at the discussion about the callback? (so I can mark it as resolved)

There is a small typo left in the code typeof !this.driver.validate === "function" this should of course have been typeof this.driver.validate !== "function". (Currently it compares 'boolean' === 'function', although this causes no errors and is always false, it is neater to have it right `). After days of testing, I couldn't find anything else.

Apart from some grammar here and there, I think it is quite stable and ready for the @next release. Thanks for the fine cooperation so far and I hope to have brought the library forward.

(I will remove my repository once the readme is updated and no longer refers to my repo for installation).

@supersnager
Copy link

@mikenicholson, @Outternet The rewrite is an awesome work. Any chance of a 5.x version release?

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