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

fastify-session integration #421

Merged
merged 18 commits into from
Jan 4, 2022
Merged

Conversation

sameer-coder
Copy link
Contributor

@sameer-coder sameer-coder commented Dec 20, 2021

This PR adds support for fastify-session.

PR includes:

  • A new SessionManager class for fastify-session
  • Tests
  • Documentation update

Closes #319
Closes #376

README.md Outdated Show resolved Hide resolved
src/AuthenticationRoute.ts Outdated Show resolved Hide resolved
src/AuthenticationRoute.ts Outdated Show resolved Hide resolved
src/Authenticator.ts Outdated Show resolved Hide resolved
@sameer-coder
Copy link
Contributor Author

Tests are working fine on local. I am investigating to see why the CI is failing

src/AuthenticationRoute.ts Outdated Show resolved Hide resolved
src/Authenticator.ts Outdated Show resolved Hide resolved
test/fastify-session/passport.test.ts Outdated Show resolved Hide resolved
@simoneb
Copy link
Contributor

simoneb commented Dec 20, 2021

about the build failing, I don't know what the problem is but I would try removing the lockfile. having a lockfile which is by its nature not portable across node versions doesn't seem like a good idea. also in general we don't use lock files in repos containing packages

@airhorns
Copy link
Member

@simoneb we had a big discussion about the lockfile here #24, I think there are good reasons for keeping it, if you look at the list of failing depfu PRs on this repo right now they're all things that might break for users or break CI without the authors understanding why.

@simoneb
Copy link
Contributor

simoneb commented Dec 20, 2021

@simoneb we had a big discussion about the lockfile here #24, I think there are good reasons for keeping it, if you look at the list of failing depfu PRs on this repo right now they're all things that might break for users or break CI without the authors understanding why.

well we have this PR and we don't understand why it's failing either :)

@airhorns
Copy link
Member

Good thing we can rule out developers using different versions of the packages dependencies as a cause then! I don't think we should randomly pull levers to see if it fixes this PR, we should look at the fail! Here's the failure message:

  ● Test suite failed to run

    test/independent-strategy-instances.test.ts:26:40 - error TS2339: Property 'get' does not exist on type 'Session'.

    26     async (request) => request.session.get('messages')
                                              ~~~
    test/independent-strategy-instances.test.ts:69:53 - error TS2339: Property 'get' does not exist on type 'Session'.
 
    69     async (request) => `messages: ${request.session.get('messages')}`
                                                           ~~~
    test/independent-strategy-instances.test.ts:112:53 - error TS2339: Property 'get' does not exist on type 'Session'.

    112     async (request) => `messages: ${request.session.get('messages')}`

My guess is that one of the changes in this PR changes the type of the request.session property to be whichever is imported last, which is probably @fastify/session, overriding the interface that used to be in place before. Those tests could be updated to cast to the right session type, or to use the new helpers being added to the session manager to access the session.

@sameer-coder sameer-coder marked this pull request as draft December 22, 2021 08:19
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amazing work!

test/decorators.test.ts Outdated Show resolved Hide resolved
test/decorators.test.ts Outdated Show resolved Hide resolved
test/run.js Outdated Show resolved Hide resolved
test/run.js Outdated Show resolved Hide resolved
test/run.js Outdated Show resolved Hide resolved
@sameer-coder sameer-coder marked this pull request as ready for review December 22, 2021 13:11
README.md Outdated Show resolved Hide resolved
Copy link
Member

@airhorns airhorns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting all this work in, just a couple more comments

test/run.js Outdated Show resolved Hide resolved
test/session-serialization.test.ts Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

package-lock.json Outdated Show resolved Hide resolved
Copy link
Member

@darkgl0w darkgl0w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sameer-coder > CI currently fails due to npm lock file version conflicts

Copy link
Member

@darkgl0w darkgl0w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@zekth zekth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✌️

@sameer-coder
Copy link
Contributor Author

I think this PR should be good to merge.

@mcollina mcollina merged commit c572f81 into fastify:main Jan 4, 2022
@mcollina mcollina mentioned this pull request Jan 4, 2022
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.

state for passport causes fastify-passport to fail Support @fastify/session out of the box
7 participants