Support for multiple distinct instances, as in Passport itself #186
Conversation
|
Thank you for your contribution! I am away for the Easter break but please nudge me if I haven't looked at it by the end of next week. |
package-lock.json
Outdated
There was a problem hiding this comment.
I don't think this should be included in these code changes?
CHANGELOG.md
Outdated
| @@ -3,6 +3,15 @@ | |||
| All notable changes to this project will be documented in this file. | |||
| This project adheres to [Semantic Versioning](http://semver.org/). | |||
|
|
|||
| ## [2.2.0] - 2023-04-06 | |||
There was a problem hiding this comment.
Let's change this to Unreleased as we don't have a definitive version number or release date yet.
| ## [2.2.0] - 2023-04-06 | |
| ## [Unreleased] |
CHANGELOG.md
Outdated
| - In addition to exporting an object that can be immediately used, | ||
| just as before, we have added an `AuthTokenRefresh` property. This is a | ||
| constructor that can be invoked to create a distinct instance, for | ||
| applications that require more than one passport instance. (Passport has the same feature.) |
There was a problem hiding this comment.
I think we can make this more concise. How about the following:
| - In addition to exporting an object that can be immediately used, | |
| just as before, we have added an `AuthTokenRefresh` property. This is a | |
| constructor that can be invoked to create a distinct instance, for | |
| applications that require more than one passport instance. (Passport has the same feature.) | |
| - Added a new named export `AuthTokenRefresh`. This is a | |
| constructor that can be invoked to create a distinct instance, for | |
| applications that require more than one Passport instance. |
|
Looking at the prototype idea, I saw your point but figured it would be more maintainable in the long run to use class syntax to express that, and that seems to have turned out well in this newest revision. In practice it's the same. Let me know what you think. I tried renaming "_strategies" to "strategies", but right away that broke tests that worked without modification before. Sure, it's a private property and the tests are a special case, but I thought "this is probably meant to be a private property, but if I can avoid maybe breaking someone else's code just by leaving a name unchanged..." and left it unchanged. I took your suggestions directly on everything else. Thanks, let me know what you think! |
|
Published as v2.2.0 |
|
Thanks so much! |
Thanks for creating and maintaining this module!
In our application, it is necessary to have separate instances of Passport itself. Passport supports this: the default export is an instance, which makes sense for most projects, but there is a "Passport" property on that instance as well. This is a constructor that can be used to make a separate and distinct instance of Passport that doesn't share any configuration with the default one (or any others).
In this PR, I've contributed the same feature for passport-oauth2-refresh. I believe the test coverage shows the functionality hasn't changed.
Thanks for taking a look, and by all means let me know if any changes are needed etc.