Skip to content
This repository has been archived by the owner on Jul 30, 2022. It is now read-only.

Tables should not always merge #12

Open
suchasaltylemon opened this issue Apr 16, 2022 · 3 comments
Open

Tables should not always merge #12

suchasaltylemon opened this issue Apr 16, 2022 · 3 comments

Comments

@suchasaltylemon
Copy link

suchasaltylemon commented Apr 16, 2022

When using Set(), you do not always want tables to merge. This restricts the data that can be stored, only allowing for extending a table, which is not always the desired behaviour.

@cxmeel
Copy link
Owner

cxmeel commented Apr 16, 2022

@suchasaltylemon could you provide a snippet/scenario where you might not want tables to merge?

BasicState treats its internal state as immutable, and you should be accessing and manipulating state using the getters/setters. The idea behind the merge functionality of SetState is to allow the mass modification of keys without having to call Set key-by-key.

@suchasaltylemon
Copy link
Author

suchasaltylemon commented Apr 16, 2022

@csqrl my mistake, i meant to be referring to the Set method, and apologies for creating this issue as I now realise that the @rbxts/basicstate module has not updated its source code to reflect updates to this repository. My issue was opened in response to old behaviour where Set() would automatically merge tables, and I had assumed that the npm repository was up-to-date.

@suchasaltylemon
Copy link
Author

@csqrl after having rechecked the behaviour I have found that the merge behaviour still does exist when calling Set() when the value parameter is a table. While this behaviour was initally added to fix problems where state would not always update, it also means that any value which is intended to act as an array can never have elements removed, and can instead only be extended. This can be problematic with any data which may be dynamically sized, such as items in an inventory.

Unless I am misunderstanding how/when to use BasicState, I believe that always merging tables can have some problems.

Also, apologies for closing this issue, I had somehow convinced myself that an older commit was the current version.

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

No branches or pull requests

2 participants