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

Fix to save session when overwrite configuration is set #30

Closed
wants to merge 3 commits into from

Conversation

justinlinz
Copy link

In the current version, if you update maxAge or expires without changing the content of the session, the session cookie does not get resaved with the new expiration. This fixes that by saving the session when the existing overwrite property is set to true.

@dougwilson dougwilson added the pr label Mar 18, 2015
@dougwilson
Copy link
Contributor

Please look into/fix why the tests are failing with this change.

@justinlinz
Copy link
Author

Thanks for sorting that at Purple Panda. Doug, anything else I can do on this one? Also, do you have a tentative date for future release?

@dougwilson
Copy link
Contributor

Sorry, GitHub does not notify me when new commits are pushed on PRs, so I didn't even know it was fixed yet :)

Looks like a default value was changed in the README and a test was deleted. These signal to me this is not a backwards-compatible change and would require a major version bump. If this is the case, I cannot promise any timeline.

Is this really a breaking change?

@dougwilson dougwilson self-assigned this Mar 20, 2015
@jlsherrin
Copy link

That's alright @dougwilson. I'm sure its not easy maintaining so many repositories.

As for the documentation, it was actually just wrong before. It looks like it was just copy/pasted from the documentation at https://github.com/pillarjs/cookies, but the code clearly sets the overwrite option to true on line 31. I think this is the correct behavior for a session cookie, but the problem I ran into is that it only gets overwritten if the contents of the cookie has changed. I don't want to manually update the cookie every time a user interacts with my application though. This is why I created the issue #31.

As for the test, I didn't actually delete the test I just moved it around it a bit. I added a sub-describe under when accessed but not changed. It appears the change in indentation marked it as being completely different. I also added another test to set the overwrite option to false. I would be willing to rewrite the tests if you'd like. I tried to follow the format of the other tests, but I don't know if I successfully achieved that.

I guess if users of the module are not expecting the cookie to be updated without a change to the cookie of the contents changing this could be considered a breaking change.

Sorry for the novel.

@dougwilson
Copy link
Contributor

Ah, thank you so much for the explanation :) Sometimes modules like this where I inherited them rather than wrote them, it is hard to me to tell right away. With that explanation, I'll try to look at it this weekend, and if it looks good, I'll make a release.

@danpaz
Copy link

danpaz commented Jun 30, 2015

+1 Anything needed to get this one released? Happy to help out if needed, would love to have this work as described above!

@dougwilson
Copy link
Contributor

Hi! I just took a look into this and this change is not valid; override's own documentation explains it's meaning, which does not mean to keep setting the session over and over again. To do that, please alter this PR to make a new option called something else.

The documentation fix, though, is valid and accepted in 81fd013

@jlsherrin
Copy link

@dougwilson, after further review I agree that overwrite does have a different intended function. So I suggest we add a new option to this library that will allow a cookie to be set even if the session is unchanged. This option should default to false for backwards compatibility of course. Since I'm not very creative I would just call it saveUnchanged.

If this is an acceptable solution. I will gladly submit a pull requests to implement it.

@dougwilson
Copy link
Contributor

Sounds fine; we can always discuss the option's name in said PR, I wouldn't worry about it here :) I tried to implement it myself when I made my last comment and found it almost impossible to use, since if the browser makes two parallel requests, and you alter the session in one request, which comes back first, the second request coming back clobbers the changes.

@dougwilson
Copy link
Contributor

Closing this PR, since the change was found to be invalid, since it was just a misunderstanding of the existing behavior. I was hoping there would be a follow-up PR, but nothing has appear so far :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants