Allow Schema fields to be visible when their parent is visible#218
Allow Schema fields to be visible when their parent is visible#218endel merged 11 commits intocolyseus:masterfrom
Conversation
…bility as its parent
| "generate-test-11": "bin/schema-codegen test-external/MapSchemaMoveNullifyType.ts --namespace SchemaTest.MapSchemaMoveNullifyType --output ../colyseus-unity-sdk/Assets/Colyseus/Tests/Editor/ColyseusTests/Schema/MapSchemaMoveNullifyType", | ||
| "generate-test-12": "bin/schema-codegen test-external/ArraySchemaClear --namespace SchemaTest.ArraySchemaClear --output ../colyseus-unity-sdk/Assets/Colyseus/Tests/Editor/ColyseusTests/Schema/ArraySchemaClear", | ||
| "prepublishOnly": "npm run build" | ||
| "prepare": "npm run build" |
There was a problem hiding this comment.
Happy to undo this if this PR is going to be merged. I needed my fork to build on install so I can use it directly from github.
|
Wow, thanks for the PR @FTWinston, I will review it very soon. I'm afraid I wouldn't like to add more complexity to StateViews so if we can change the default behaviour to be as you would expect would be better. We are also considering moving away from decorators in the future (see #213). |
|
Hey, no worries. If the default behaviour changed that would be awesome, but I didn't want to start with suggesting that 😅 If you have a schema in a state tree with no Once you're within a schema decorated with I think that could be achieved by discarding my changes and just removing the If you want to go with an approach like this, I'm happy to change this PR, or just close it if that's simpler. |
|
I think what you proposed here is what makes more sense! I only see advantages :)
Would you mind adapting the PR? 😇 I'd also suggest to move the tests to |
|
Gladly, thanks for the tip |
|
Ok, there you have my favourite kind of PR: one that just removes a line of code. |
|
Hi @endel friendly bump on this, since the requested change has been applied (the one-line removal of parentIsCollection + tests moved to StateView.test.ts). Sharing some real-world signal in case it helps: we're carrying this exact patch in production for an MMORPG on colyseus 0.17 + @colyseus/schema 5.0.3. Our Character schema lives inside a Would love to drop the pnpm patch on the next 5.x release. Happy to share the regression test if it'd be a useful addition to StateView.test.ts. |
|
Thanks for the heads-up @anaibol, and thank you so much for fixing the PR @FTWinston 👏 I forgot to merge it! More tests are always welcome if you can send another PR for this that'd be a good addition @anaibol 💙 Cheers! |
Brings 4.0.21 (PR #218) and prior 4.x fixes into 5.0. Conflicts resolved: - package.json: keep version 5.0.3 (HEAD). - CHANGELOG.md: keep 5.0.x-only changelog (HEAD). - .github/workflows/npm-publish.yml: keep 5.0's branch-aware npm tag / prerelease logic (master/@latest, 5.0/@next). - tsconfig.test.json: keep HEAD's empty exclude list. - src/types/HelperTypes.ts: keep HEAD's ToJSON (already incorporates the #222 fix more carefully via ToJSONRequiredKeys/ToJSONOptionalKeys plus .optional() support). - src/encoder/ChangeTree.ts: drop master's old inline parent-chain + _checkFilteredByParent block — already refactored on 5.0 into changeTree/parentChain.ts and changeTree/inheritedFlags.ts. - test/StateView.test.ts: keep HEAD's "isNew fast path" suite AND port master's three nested-Schema visibility tests under a new "nested Schema parent visibility (#218)" describe. #218 functional fix ported into 5.0: - src/encoder/changeTree/inheritedFlags.ts: drop the `parentIsCollection` constraint from `isVisibilitySharedWithParent` so a nested Schema field inherits visibility from a @view-gated parent without needing to be wrapped in an ArraySchema. Matches the new test expectations. All 644 tests pass. Assisted-by: Claude Opus 4.7
Existing behaviour
When using
@viewwith nestedSchemaclasses, visibility can be unintuitive. Currently:Schemafields inside it do not become visible (unless they are also decorated with@viewand explicitly added to the client's view.if there's a
parentthat's visible to a client:Then a
childwithin it will be included in the encoded state, but all of its fields will beundefined:That is, unless that child is held in an
ArraySchema:Placing the
childinside anArraySchemadoes propagate field visibility.It seems as if
ArraySchemaitems inherit visibility from the parent of theArraySchema. I'm not certain whether that's intentional, but I am finding it helpful!What this PR changes
This PR removes the restriction that child schemas will inherit their parent's visibility only if the parent is a collection. This means that visibility of fields on the
Childclass mentioned above behave the same, regardless of whether its contained in anArraySchemaor not:if the parent is visible, the child is visible and its fields are encoded normally.
Motivation
This avoids the need to wrap single nested objects in
ArraySchemapurely to get visibility propagation.I’m using this for several small nested types (e.g., a
Cooldowntimer object), and while my state could obviously be structured differently, this change means removes the need to occasionally "fight" against the view system.