-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement property migration #268
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -16,7 +16,7 @@ jobs: | |||||
|
||||||
strategy: | ||||||
matrix: | ||||||
rust_version: ["1.64.0"] | ||||||
rust_version: ["1.65.0"] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
steps: | ||||||
- uses: actions/checkout@v1 | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I understand correctly, migrations swap out the old property for the new one in deserialization. It doesn't actual migrate the model file, it will still have the old property. I think this would be useful information to include. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
# How to Fix a New Property Added by Roblox | ||
When Roblox introduces new properties, usually tools like Rojo can use them without any additional changes. Sometimes, though, properties are added with different names, multiple serialized forms, or aren't listed at all in the reflection dump that Roblox gives us. | ||
When Roblox introduces new properties, usually tools like Rojo can use them without any additional changes. Sometimes, though, properties are added with different names, multiple serialized forms, need to be migrated to a new property, or aren't listed at all in the reflection dump that Roblox gives us. | ||
|
||
This document describes some common scenarios, and what work needs to happen to fix them. | ||
|
||
|
@@ -91,6 +91,32 @@ Change: | |
AliasFor: MaxPlayers | ||
``` | ||
|
||
## Roblox added a new property, but it's a migration from an existing property, and the existing property no longer loads | ||
Sometimes Roblox migrates an existing property whose type is too constrained to a new property with a more flexible type. | ||
|
||
Without special handling, this can cause problems for binary files because when old and new models are mixed together, the binary serializer must add the new property to the old models. Without special instruction, it'll just add the default value. This can result in weird behavior like old text UI all having the Arial font, because the default value of a new property took priority. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
What do you mean by "mixing" models? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Is this talking about |
||
|
||
To fix this, we need to write a migration (in Rust) and apply it is as a patch (using database patch files). | ||
|
||
First, add your migration to the PropertyMigration enum in [`rbx_reflection/src/migrations`][migrations]. The migration should be named after the properties it's migrating. For example, migrating from `Font` to `FontFace` would be named `FontToFontFace`. | ||
|
||
Next, add code to convert from the old property's type to the new property's type. This code should be a new match arm in the `perform_migration` function in [`rbx_reflection/src/migrations`][migrations]. | ||
|
||
Finally, add a patch in the [patches](patches) folder. This patch should change the old property's serialization type to `Migrate`, specifying the new property name and the migration name. | ||
|
||
For example, the patch for fonts looks like: | ||
```yaml | ||
Change: | ||
TextLabel: | ||
Font: # Property we're migrating *from* | ||
Serialization: | ||
Type: Migrate | ||
Property: FontFace # Property we're migrating *to* | ||
Migration: FontToFontFace # Migration we're using | ||
``` | ||
|
||
If this property is present on multiple classes, you may need to specify the Serialization change for multiple properties on multiple classes. For example, the `Font` property is present on `TextLabel`, `TextButton`, `TextBox` without being derived from a superclass, so the real patch is approximately 3 times as long since it needs to be applied to each class. | ||
|
||
## Roblox added a new property, but modifying it from Lua requires a special API | ||
Sometimes a property is added that cannot be assigned directly from Lua. | ||
|
||
|
@@ -148,4 +174,5 @@ These pull requests outline how we implemented support for Attributes in rbx-dom | |
|
||
[rbx-dom]: https://github.com/rojo-rbx/rbx-dom | ||
[patches]: https://github.com/rojo-rbx/rbx-dom/tree/master/patches | ||
[custom-properties]: https://github.com/rojo-rbx/rbx-dom/blob/master/rbx_dom_lua/src/customProperties.lua | ||
[custom-properties]: https://github.com/rojo-rbx/rbx-dom/blob/master/rbx_dom_lua/src/customProperties.lua | ||
[migrations]: https://github.com/rojo-rbx/rbx-dom/blob/master/rbx_reflection/src/migration.rs |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
Change: | ||
ScreenGui: | ||
IgnoreGuiInset: | ||
Serialization: | ||
Type: Migrate | ||
Property: ScreenInsets | ||
Migration: IgnoreGuiInsetToScreenInsets |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
Change: | ||
TextLabel: | ||
Font: | ||
Serialization: | ||
Type: Migrate | ||
Property: FontFace | ||
Migration: FontToFontFace | ||
TextButton: | ||
Font: | ||
Serialization: | ||
Type: Migrate | ||
Property: FontFace | ||
Migration: FontToFontFace | ||
TextBox: | ||
Font: | ||
Serialization: | ||
Type: Migrate | ||
Property: FontFace | ||
Migration: FontToFontFace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.