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

[Docs] Document pass-by-reference pitfall for defaulted values #1236

Closed
MaddyGuthridge opened this issue May 24, 2024 · 2 comments
Closed

[Docs] Document pass-by-reference pitfall for defaulted values #1236

MaddyGuthridge opened this issue May 24, 2024 · 2 comments

Comments

@MaddyGuthridge
Copy link

MaddyGuthridge commented May 24, 2024

I just spent about half an hour trying to figure out why data was showing up in places it shouldn't. It turned out that my defaulted values were being passed by reference, meaning that any modifications to one instance would affect all current and future instances created from the same struct.

// BAD: array is passed by reference
const MyType = defaulted(array(string()), []);

const instanceA = create(undefined, MyType);

instanceA.push("hello");

const instanceB = create(undefined, MyType);
console.log(instanceB);
// [ "hello" ]

While this makes sense in hindsight, it was pretty painful to debug (especially since my case had a significantly more complex data structure).

As such, I think this project would be improved if this pitfall was documented. I've created a PR that adds this documentation.

Additionally, I noticed that the type signatures for defaulted allow any as the default value. Is this intentional? I imagine that stricter types would allow for significantly improved safety, helping users to avoid this issue (eg only accepting a plain value if the value is a type that cannot be modified).

@MaddyGuthridge
Copy link
Author

Wait hang on a second my example doesn't work........ why on earth did changing it to a function fix this for my bug?

@MaddyGuthridge
Copy link
Author

Ok looks like the issue is more complex than I thought, I'll close this issue and the related PR and open a new issue that describes the problem better.

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 a pull request may close this issue.

1 participant