Refactor ServerTableReplicator and bump version#51
Conversation
Normalize formatting and types across ServerTableReplicator, add validation and safety checks, and bump package version to 0.2.8. Key changes: - Consistent spacing/indentation, minor whitespace and table/type annotation formatting. - Strengthened config validation in ServerTableReplicator.new: validate/auto-register class token strings, assert parent/replication target rules, validate Client table, and normalize TableManager handling. - Added safety guard in internal ConnectToSignal wrapper to skip firing when the TableManager (or STR) is destroyed. - Improved replication target handling and clarified Start/Stop replication flows (EnsureIsPlayerArray, TargetsToArray, AssertReplicationTargets). - Various API renames/aliases kept; small restructuring of creation/replication logic for correctness when changing parents (creation data and client notifications). - Constructed ServerTableReplicator.All/None with table literal style. - Bumped lib/tablereplicator/wally.toml version to 0.2.8. These changes improve robustness and readability without changing external behavior except for added safeguards and stricter validation.
There was a problem hiding this comment.
Pull request overview
Refactors ServerTableReplicator for more consistent formatting/types, adds additional validation/safety checks around initialization and signal replication, and bumps the package version to 0.2.8.
Changes:
- Bumped
wally.tomlpackage version to0.2.8. - Normalized formatting/types and strengthened config validation in
ServerTableReplicator.new(token/targets/parent/client/table-manager handling). - Added a destruction guard in the internal TableManager signal connection wrapper to avoid replicating after teardown.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/tablereplicator/wally.toml | Version bump to 0.2.8. |
| lib/tablereplicator/src/Server/ServerTableReplicator.luau | Refactor + stricter validation + replication safety guard changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| local existingToken = table.find(USED_CLASS_TOKENS, tokenName) | ||
| if existingToken then | ||
| error(`Class Token '{tokenName}' already exists. Created at: \n{existingToken.Creation}`) | ||
| end |
There was a problem hiding this comment.
table.find(USED_CLASS_TOKENS, tokenName) returns the index (number), but the error message tries to read existingToken.Creation, which will throw while building the error string. Consider storing tokens/creation traces in a dictionary (e.g. name -> traceback) or storing the full token objects in USED_CLASS_TOKENS, and use that stored creation info in the duplicate-token error.
| BaseTableReplicator.new { | ||
| TableManager = config.TableManager, | ||
| Tags = config.Tags, | ||
| IsTopLevel = parent ~= nil, |
There was a problem hiding this comment.
IsTopLevel is being passed as parent ~= nil, which is the opposite of the documented meaning (top-level == no parent). Even though BaseTableReplicator.new currently doesn't use this field, this inverted value is misleading and could become a real bug if the base constructor starts honoring it. Recommend removing the field entirely or setting it to parent == nil.
| IsTopLevel = parent ~= nil, | |
| IsTopLevel = parent == nil, |
| if not config.TableManager then | ||
| warn("No TableManager was provided. Creating an empty TableManager") | ||
| config.TableManager = TableManager.new {} | ||
| elseif config.TableManager.ClassName ~= TableManager.ClassName then |
There was a problem hiding this comment.
config.TableManager.ClassName is accessed without first ensuring config.TableManager is a table; if a non-table value is passed (this function already handles nil and attempts to coerce non-TableManager inputs), this will error with a generic 'attempt to index' message before the more helpful TableManager validation later. Add a typeof(config.TableManager) == "table" (or similar) guard before reading .ClassName.
| elseif config.TableManager.ClassName ~= TableManager.ClassName then | |
| elseif type(config.TableManager) ~= "table" or config.TableManager.ClassName ~= TableManager.ClassName then |
Normalize formatting and types across ServerTableReplicator, add validation and safety checks, and bump package version to 0.2.8.
Key changes:
These changes improve robustness and readability without changing external behavior except for added safeguards and stricter validation.