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

created Options interface #33

Closed
wants to merge 1 commit into from
Closed

Conversation

jpfluger
Copy link

@jpfluger jpfluger commented Jan 27, 2018

Options interface exposes the private options struct without breaking code, so this is backwards-compatible. Only getters are available on the interface. set functions could be added later, if desired. The interface is available through the manager.Opts() function. Added a single test function that compares against defaults.

There are two additional commits associated with this pull request. This was actually unintended b/c I began the pull request before the 2nd two commits were added but Github automatically added them to this request. I can try splitting out, if you want.

All three commits are backwards-compatible with the defaults.

Re. saved property to session: helps the backend data store determine stale records. Also required by TouchWithInterval() to know if Touch() should occur. In order to preserve strict backwards compatibility, I added a manager.UseSaved() function. If the opts.useSaved value is true, only then will the date be saved in the sessions store.

Re. TouchWithInterval: limits amount of unnecessary writes to back end data store. This would close #32.

I have an an echo middleware example but will wait to commit and have that conversation after we can get past these. Thanks!

@jpfluger jpfluger changed the title created Options interface created Options interface; added saved date property to session; added TimeWithInterval Jan 27, 2018
@jpfluger jpfluger changed the title created Options interface; added saved date property to session; added TimeWithInterval created Options interface; added saved date property to session; added TouchWithInterval Jan 27, 2018
@alexedwards
Copy link
Owner

Could you split the Opts() commit out from the other two? I'm happy to merge that one, but want to think about the other two.

@jpfluger jpfluger force-pushed the master branch 2 times, most recently from 52a6467 to 5a96c03 Compare January 27, 2018 21:54
@jpfluger jpfluger changed the title created Options interface; added saved date property to session; added TouchWithInterval created Options interface Jan 27, 2018
@jpfluger
Copy link
Author

I removed the last two commits. Much better. FYI, I used this gist successfully. I had first tried git rebase -i that worked locally but not upstream.

@jpfluger
Copy link
Author

I'll create a new issue to discuss saved. I'm not as warm on it since I have a better understanding of checking the IdleTimeout from middleware.

I'm dropping the request for TouchWithInterval completely.

@jpfluger
Copy link
Author

This pull request is old. How about I withdraw this request until v2 comes out and can re-examine? Or leave it here for historical purposes?

As is the case with many gophers, go.mod has forced me to re-address my code-base. (sigh) Which means I'm refreshing my local fork of scs with new commits from master and re-examining if I can merge back into master. I want to but don't know if it's possible - I may have diverged too far from master in my local copy. Once I get my local fork refreshed and tested, I'll post in a new branch echo.

Since this original pull-request, I've come to define session options within a configuration file and feed them into scs. This commit doesn't reflect that.

    "sessionOptions": {
      "domain": "example.com",
      "httpOnly": true,
      "idleTimeoutMinutes": 0,
      "lifetimeMinutes": 1440,
      "name": "session",
      "path": "/",
      "persist": false,
      "secure": false,
      "autoSave": true
    }

@jpfluger
Copy link
Author

V2 obsoletes this request. 👍

@jpfluger jpfluger closed this Apr 30, 2019
@jpfluger jpfluger mentioned this pull request Apr 30, 2019
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.

Add support for Touch Interval Rate
2 participants