JLuboff / connect-mssql-v2

MS SQL Server session store for Express Session
MIT License
5 stars 7 forks source link

feat: add preRemoveCallback #73

Closed bradtaniguchi closed 3 months ago

bradtaniguchi commented 4 months ago

Close #72

bradtaniguchi commented 4 months ago

I think I can try to add in some.

I'll have to review how to setup a full/accurate test environment on my WSL environment tho.

Might be doing the tests a little blind otherwise 😅

bradtaniguchi commented 4 months ago

I added a test case that follows a similar setup (but uses promises to keep things flat rather than nested callbacks), but I don't have a MSSQL database around to help run against so can't verify it.

edit let me just signup for an azure free tier account real quick to help test these changes out.... 😉

bradtaniguchi commented 4 months ago

Ok I got an SQL instance from Azure (thx free tier), I confirmed the tests is working/going through from my end. I also ran a smoke check against the test (instead of checking the callback was called once, I changed it to 10) and confirmed the test fails.

Also now that I have an SQL server to test against, I should be able to help debug/clean up this repo if needed, as I can finally test my changes against the test suite ^.^

JLuboff commented 4 months ago

Thanks for adding the test! Looks good to me (I ran locally). I will say...I don't know what I was thinking calling the variable cbed (callback ended??? I have no idea). After this PR gets merged I'll do a cleanup PR (and update deps). Glad you got SQL Server running :D I run a local docker build of SQL Server (express). Works well and only spin it up when I need to. I seem to recall pulling it down from Microsoft.

bradtaniguchi commented 4 months ago

Thanks for adding the test! Looks good to me (I ran locally). I will say...I don't know what I was thinking calling the variable cbed (callback ended??? I have no idea). After this PR gets merged I'll do a cleanup PR (and update deps). Glad you got SQL Server running :D I run a local docker build of SQL Server (express). Works well and only spin it up when I need to. I seem to recall pulling it down from Microsoft.

Yea I have a new laptop that I can actually work on (previously was doing some convoluted stuff on a chromebook). Getting a docker setup running could also help test this stuff out if needed 👍🏼

JLuboff commented 3 months ago

@bradtaniguchi Not sure if you were waiting to merge this since it was failing the checks. I cleared that (it was complaining due to line length in the README which doesn't matter). I'll have to spend a little time configuring Codacy better or removing it. However, I realized one thing. Need the version updated (minor?) in package.json and update the changelog. Can be a separate PR, this PR, or I can handle it. You decide. Will than release (I think I'll need to? I can't recall if we have you setup for permissions in npm or not).

bradtaniguchi commented 3 months ago

I've been meaning to get back to this, I can fix the build/bug issues in a week or so.

You can do the release, as I'm not sure how the NPM package setup is done off the top of my head.

JLuboff commented 3 months ago

Piggy-backed on your PR for the changelog and semver change so it can all be kept together.

bradtaniguchi commented 3 months ago

Thanks @JLuboff for finishing this one up 👍🏼