Updates for Ref properties specified with attributes are always `null` in `/api/subscribe` responses

kennethloeffler commented 4 months ago

To reproduce:

Run rojo plugin install
Run rojo serve on the ref_properties test project
Open a fresh baseplate in Roblox Studio, enable trace logging in the Rojo plugin, and click Connect
Make any change to ref_properties' default.project.json (I added a newline)

Observe the output, and notice that the subscribe response contains null for all the Ref properties specified with attributes in ref_properties' default.project.json:

{
"sessionId": "d264957a-76ce-486b-8df6-7151de8da0c1",
"messageCursor": 2,
"messages": [
{
  "removed": [],
  "added": {},
  "updated": [
    {
      "id": "516ae8375fd349b4a61ac47529c969cf",
      "changedName": null,
      "changedClassName": null,
      "changedProperties": {
        "Value": null
      },
      "changedMetadata": null
    },
    {
      "id": "1aee8e1cc88e674f9917cc95e9d56bf6",
      "changedName": null,
      "changedClassName": null,
      "changedProperties": {
        "Value": null
      },
      "changedMetadata": null
    },
    {
      "id": "5e3e89750a0826e8d5d83b1ee86cad1a",
      "changedName": null,
      "changedClassName": null,
      "changedProperties": {
        "PrimaryPart": null
      },
      "changedMetadata": null
    },
    {
      "id": "04b43e42134d58c42408ff2b9ba1c9a4",
      "changedName": null,
      "changedClassName": null,
      "changedProperties": {
        "Value": null
      },
      "changedMetadata": null
    }
  ]
}
]
}

I noticed this behavior in some test failures after #843 was merged: https://github.com/rojo-rbx/rojo/actions/runs/9605402645/job/26492956529#step:7:762. This makes it impossible for the Rojo plugin to live update a Ref property, could cause problems once rbx_dom_lua gains better ability to represent optional values, and can currently cause test failures on macOS due to seemingly inherent noisiness in FSEvents.

One reason this could happen is because deferred Ref property application appears to not influence the returned result of patch application (i.e. PatchApplyContext.applied_patch_set), but rather only influences the tree: https://github.com/rojo-rbx/rojo/blob/7e2bab921aa71f76d07d0424e4bb4064e8b7c995/src/snapshot/patch_apply.rs#L246-L299

It will also be worth investigating patch computation, to see if Ref properties specified via attributes are erroneously detected as removed there.

Dekkonot commented 4 months ago

I'll be completely honest: I'm not sure what part of the code I need to touch for this. I can do some backtracing to work out what part, but it might be easier to just ask if you know. So, what part of the code would I need to look in for this? Outside the patch application code, I mean.

kennethloeffler commented 4 months ago

The root cause seems to be that ref properties specified via attributes are not included in snapshots of the file system, i.e. when we get here https://github.com/rojo-rbx/rojo/blob/3ca975d81dfd6f06e024dec5a87bedf5f796dfc2/src/snapshot/patch_compute.rs#L120-L141 visited_properties is only populated with properties directly from the snapshot. Ref properties specified via attributes will not exist there, and will be detected as having changed to None in the second loop

why this happens makes sense - it's just that resolving ref properties here will require a slight design change

rojo-rbx / rojo

Updates for Ref properties specified with attributes are always `null` in `/api/subscribe` responses #929