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

consider not exposing data by reference #4

Open
chrisguttandin opened this issue Dec 5, 2020 · 2 comments
Open

consider not exposing data by reference #4

chrisguttandin opened this issue Dec 5, 2020 · 2 comments

Comments

@chrisguttandin
Copy link
Collaborator

This issue is meant to cover the discussion that started here.

Currently the exposed data structures are the same that are used internally. It would be possible that a user of this library modifies the data not realizing that this might change the behavior.

Anyway it's more a theoretical issue for now since we are the only consumers of this library. :-)

@kettanaito
Copy link
Member

This is a great suggestion, @chrisguttandin. Although we are the only consumer now, that shouldn't be the main pivot of architecture decisions.

Do you suggest we serialize the internal cookies representation in some way before returning it to the consumer?

@chrisguttandin
Copy link
Collaborator Author

We could for example change ...

const originCookies = this.store.get(requestUrl.origin) || new Map<string, Cookie>()

... to something like ...

const originCookies = new Map(this.store.get(requestUrl.origin)?.entries())

But even then the cookie objects would be passed by reference.

However a side effect of that change would be that the cookies stored in document.cookie wouldn't end up in the store anymore. I think they do now because we just add them to the internal Map here: https://github.com/mswjs/virtual-cookies/blob/master/src/CookieStore.ts#L78.

Having all that said, I think we can also put this on hold for now. And wait until someone else starts using that library.

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

No branches or pull requests

2 participants