Implement property migration

Corecii commented 1 year ago

This is a fixed version of https://github.com/rojo-rbx/rbx-dom/pull/253 because its git history was cursed and I didn't want to force push or anything else weird to fix it.

Preview

This PR allows us to specify migrations using the patches files and a Variant to Variant converter in the actual Rust code. This is better than having the Rust code do hard-coded checks against property names and such without being super complex to implement or think about. Writing new migrations this way is easy!

The patch files look like this:

Change:
  TextLabel:
    Font:
      Serialization:
        Type: Migrate
        Property: FontFace
        Migration: FontToFontFace

I've chosen to have migrations not be performed if the target property already exists. This matches Roblox's behavior where new properties always take priority. I figure making priority work both ways would be complex, and since backwards priority isn't known behavior of any present-day properties, it's unnecessary complexity. There's room to expand this in the future if we need to.

Why do we need this?

Two new properties -- FontFace and ScreenInsets -- are migrated from old properties. When both the old and new properties are present, Roblox always prefers the new properties.

The Roblox binary format requires that all objects of the same class have the same properties present. When mixing old models and new models into the same file using Rojo or Remodel, old models get assigned defaults for the new properties, which then take priority over the old properties.

This has two clear affects that users are actively experiencing in "preview" builds of Rojo and Remodel:

Mixing old and new models with UI via rojo build or Remodel scripts results in all old text UI having the Arial font.
Mixing old and new models with ScreenGuis via rojo build or Remodel scripts results in all old ScreenGuis losing their IgnoreGuiInset value.

The only work-around for users right now is to open their models in Roblox and re-save over them to get Roblox to do the migration. This is not a reasonable ask of a Rojo fully-managed project that may have hundreds of models.

As time goes on, more properties will get migrations like this and maintaining a fully-managed Rojo codebase will become an unreasonable task.

Implementing these migrations on the rbx-dom side is quite easy. If we don't do it, either someone else will add this feature themselves or people just won't do fully-managed Rojo due to the high risk of breakage.

Changes in this PR

Adds a new type of PropertySerialization, Migrate, where you specify the new property name to migrate to and the name of the migration.
Adds named migrations, which convert from one Variant to another Variant.
Changes deserializer code to respect the Migrate type.
Adds tests for these new migrations.
Adds documentation instructing how to add a new migration.

Corecii commented 1 year ago

I'd like to give this more time and attention, but I have a lot going on right now. I feel like this code can be improved, but any PR is a good start for now, right?

Dekkonot commented 1 year ago

...I should have thought about this beforehand but I just merged #249 which changes the format of patches a bit and swaps us over to using a polished version of generate_reflection named rbx_reflector. You'll need to use that tool in this PR instead because it'll get archived Soon(tm).

Beyond having a better UX, we now no longer need to add properties to the database, only modify their serialization. That meshes well with migrations and should hopefully make it reasonable to have those migrations live as patches.

Dekkonot commented 1 year ago

Work on this has been superseded by PR #283! Sorry that we ended up bungling this, I'll try to be more mindful of how the order things get merged in the future.

rojo-rbx / rbx-dom