Skip to content

feat: add option to skip write when possible#10

Open
roziscoding wants to merge 6 commits intomainfrom
feat/proxy
Open

feat: add option to skip write when possible#10
roziscoding wants to merge 6 commits intomainfrom
feat/proxy

Conversation

@roziscoding
Copy link
Copy Markdown
Contributor

@roziscoding roziscoding commented Mar 24, 2025

I need help in figuring out other possible edge cases or caveats so we can add tests for them and, ultimately, document them.

Usage:

composer.use(session({ trackChanges: true })

Closes #3

@roziscoding roziscoding self-assigned this Mar 24, 2025
@roziscoding roziscoding requested a review from KnorpelSenf March 24, 2025 12:58
Comment on lines +1 to +9
/**
* @internal Symbol used to track if the object has been mutated
*/
export const _MUTATED = Symbol("mutated");

/**
* @internal Symbol used to track if the object is being tracked
*/
export const _TRACKED = Symbol("tracked");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we leave them internal, or is there a good reason to export them?

Suggested change
/**
* @internal Symbol used to track if the object has been mutated
*/
export const _MUTATED = Symbol("mutated");
/**
* @internal Symbol used to track if the object is being tracked
*/
export const _TRACKED = Symbol("tracked");
/**
* @internal Symbol used to track if the object has been mutated
*/
const _MUTATED = Symbol("mutated");
/**
* @internal Symbol used to track if the object is being tracked
*/
const _TRACKED = Symbol("tracked");

Copy link
Copy Markdown
Contributor Author

@roziscoding roziscoding Apr 6, 2025

Choose a reason for hiding this comment

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

We use them for testing.
I don't know if there's another way of exposing these to tests but not to the public API.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we can use Symbol.for then

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Or just make sure we only export things internally but don't leak it to package consumers :)

return object as Mutatable<T>;
}

const proxyCache = new WeakMap();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How hard would it be to tie the cache to a property session instead? That would feel like a more obvious strategy to manage the lifetime of these objects in memory

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think it'd be that hard, but it's not obvious to me why we'd want to do it, sorry.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I find it hard to reason about the lifecycle of memory when there are different references to the objects, and only some of them prevent it from being garbage-collected. This is especially so if there is an obvious way to structure that code such that the memory is directly tied to the stack, which makes it very obvious how long the objects live. But if you have strong opinions about keeping it in a WeakMap, then I'll be able to wrap my head around it :)

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.

feat: track session changes via deep proxification

2 participants