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

[Outternet]

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]

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]

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

[Outternet]

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]

People with intellij could not see the library properly, I fixed this problem, but it does not hurt to test with some more IDEs, currently only intellij and visual studio enterprise are tested.

[Outternet]

I tested it more extensively with nestjs/express, so far everything works stable. This is of course in addition to the production app and 102 unit tests.

[mikenicholson]

This looks great. Reading through the code now. I really appreciate the thorough tests and documentation updates.

[Outternet]

@mikenicholson It's good to hear from you, I'm glad you're excited. I have incorporated some more internal feedback and improved some grammar. In addition, the tests now have a 100% line converage.

I would love to hear your feedback on the current implementation, but I am glad you are open to the idea of a major update.

[Outternet]

I focused a bit more on improving the documentation and supporting older versions of node and typescript without affecting the new version (the library is probably still often used with older versions).

[mikenicholson]

I largely expect to merge this as-is. It's in great shape. No major concerns and I really appreciate the effort dedicated to providing a clear upgrade path and docs to users.

Can we remove the dist/ directory in this PR, add it to the .gitignore and set up a .npmignore to dist/ is not excluded from npm packaging?

I'm currently working through a few CI issues and a test failure under older versions of node (<=14) in a branch. Once I have solutions to those issues I think we'll be in good shape to merge.

[Outternet]

Awesome, The dist/ directory is already included by npm because of the files property in the package.json (and the lib directory is ignored, if you install it now you only get the needed files), yes the dist should indeed be excluded from git, this was mainly added so that our function tests could be easier done from github.

Regarding test conditions, I currently have an engine restriction of node >= 12 down, this can be bypassed by the user during installation but then fallback.cjs must be used and the platform system will not work outside of jsonwebtoken. Strange that the jose driver does not pass the key properly (fortunately only a test) if you need help I am available.

Finally, I think it would be wise to close all open issues, since they don't really add anything anymore and many of them are out of scope; if they are still relevant, they can be reopened.

[Outternet]

the error is in the createSecret at the jose driver test, the signuture of createSecret has been changed (I used the new version) in the code itself this fix has already been applied previously. change createSecret("foobar") to createSecret(Buffer.from("foobar")) and your good to go.

On another note, I have put my name and copyright at the bottom of my repo. I will be honored if that is allowed to remain, but I do want you to make that choice consciously.

[Outternet]

We should determine whether it will be released immediately or first as a passport-jwt@next version, given the importance of autentification. In doing so, we need to contact passportjs and nestjs to update their documentation as well.

https://docs.nestjs.com/security/authentication https://www.passportjs.org/packages/passport-jwt/ (seems to be just the readme)

Most of the documentation is pretty solid, but I'm not a native english speaker and had trouble wording the error messages correctly, I'd appreciate it if you'd check them again for grammer issues.

[mikenicholson]

the error is in the createSecret at the jose driver test, the signuture of createSecret has been changed (I used the new version) in the code itself this fix has already been applied previously. change createSecret("foobar") to createSecret(Buffer.from("foobar")) and your good to go.

I've confirmed that fix on the branch, we can apply it to this PR along with the CI changes once merged.

On another note, I have put my name and copyright at the bottom of my repo. I will be honored if that is allowed to remain, but I do want you to make that choice consciously.

I absolutely want to ensure you receive credit for this major contribution. Let me just do a quick survey of some other projects, I think contributors files are more common, but given the scope of this rewrite I think the spot on the README makes sense right now.

We should determine whether it will be released immediately or first as a passport-jwt@next version, given the importance of autentification.

I think given the scope of the change and the remaining work required to get this ready to release as a new major version, I'd like to merge this into a new 'development' branch (not master). This will give me time to get the CI updated, make any final documentation updates, etc. We can begin releasing to NPM from this branch under the passport-jwt@next tag as soon as we merge and using version numbers like v5.0.0-rc.1 to tag in the repo. Once v5 is ready to release, I'll merge the development branch to master and release to npm@latest

There are two reasons for using a development branch rather than master:

1. This will give external stakeholders some time to try the new version from the official NPM repo and provide feedback.
2. The github repo will continue showing the code and readme for v4.x.x by default. In the past, when I have committed major changes to master before releasing, it results in a lot of confusion and GitHub issues opened up because people expect the GitHub repo to reflect the current NPM release.

In doing so, we need to contact passportjs and nestjs to update their documentation as well.

I see you've already opened the issue with Nestjs. They raised some issues about backwards compatibility that I think might be justified. I'll take a look at what we can do to make passport-jwt/auto more backwards compatible. Again this work can happen on the development branch and shouldn't hold up this PR. I don't think its reasonable to never expect breaking changes, but given the wide audience, providing the smoothest upgrade path possible is worthwhile. In the meantime they can always specify a version in their tutorial to decouple their doc changes from our release timing.

The passportjs docs pull the README directly from either NPM or github (I think). I've never contacted them about updates and the README hosted there seems to update relatively quickly.

Most of the documentation is pretty solid, but I'm not a native english speaker and had trouble wording the error messages correctly, I'd appreciate it if you'd check them again for grammer issues.

I gave these a quick review. Just minor tweaks. Your English is great!

[Outternet]

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]

@mikenicholson you still there?

[mikenicholson]

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.

[Outternet]

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]

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