fix: clone session data on .set()

[kaytwo]

Changes

• Fixes bug / clarification: session.set is pass by reference, leading to edits after calling .set to be persisted #12881 by saving a clone of the set()'d object at time of set()
• Refactor unflatten and stringify so that the stringify check works for URL objects

I chose to use parse(stringify(value)) instead of anything like structuredClone or trying to get fancy with the store to ensure correctness, the extra parse should not be a significant performance issue unless people start storing pathologically large session data.

Testing

Added a new test in sessions.test.js that fails on main and succeeds on this branch.

That test could probably use a helper function to maintain a "cookie jar" for session, but I don't quite know the cleanest way to do that.

Docs

No update necessary, just a bugfix. However if the powers that be decide #12881 is wontfix, the docs definitely need to be updated to remind users to clone their objects before calling set, or be careful with updating them after doing so.

[changeset-bot]

🦋 Changeset detected

Latest commit: 7de535b

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[codspeed-hq]

CodSpeed Performance Report

Merging #12883 will not alter performance

_{Comparing kaytwo:main (7de535b) with main (73a0788)}

Summary

✅ 4 untouched benchmarks

[ematipico]

Looks good to me

[ascorbic]

Thanks for this. The issue with the missing .astro/session dir could be a real bug, so it would be better to fix it than work around it in the test. If it's not (maybe it just needs the parent dir creating) then could you try to make the tests behave closer to production by just creating the .astro dir so we're still ensuring it creates the session dir.

[ascorbic]

Ah, my mistake. We can't use fs there. I think it's ok to revert to your previous version.

[kaytwo]

reverted. I added a different "ensure the directory exists" check that is only a little bit of a hack. also added "/.astro" to .gitignore as both manually and turbo running the tests creates that directory in various places, and I didn't see anywhere else (e.g. being used as part of a test fixture) so that should be safe.

[ascorbic]

Can you split this into two chanegsets. You can run pnpm changeset again to add another one.

[ascorbic]

Great work. Thanks for tracking down all the issues. Just a single request with the changeset and we can merge this.

[ascorbic]

Great! Thanks a lot for fixing these two.