search

rojo-rbx / rbx-dom

Roblox DOM and (de)serialization implementation in Rust
MIT License
113 stars 46 forks source link

#332 breaks compatibility #335

Closed kennethloeffler closed 1 year ago

kennethloeffler commented 1 year ago

332 is breaking for users who use IgnoreGuiInset or Font properties in Rojo project.json files or Lune scripts. Current releases serialize these properties, but upcoming releases will not. BrickColor/brickColor are not affected because in current releases, they are aliases and therefore do not serialize.

It'd be really easy to add warnings to these places to at least notify users when they try to serialize migrated properties: https://github.com/rojo-rbx/rbx-dom/blob/7b3e6fb84ceb3b02c5042d17a373f7cd42bec83b/rbx_binary/src/serializer/state.rs#L304-L318 https://github.com/rojo-rbx/rbx-dom/blob/7b3e6fb84ceb3b02c5042d17a373f7cd42bec83b/rbx_xml/src/core.rs#L53-L62

Alternatively, we could maintain compatibility by performing migrations during serialization, but that is a lot more effort, and it's not clear whether it's worth it since we don't know how many people will break, and in the future, proxy properties will render it moot.

Dekkonot commented 1 year ago

As annoying as it is, I think the correct path is probably to run migrations during serialization as well. I don't love doing that because like you said, it's work we're going to immediately make obsolete, but the other option is pushing a release that we know will break things, which I feel is worse.

kennethloeffler commented 1 year ago

Cool, I'll get started on it right now and try to get it finished tonight. Pray for me, our serializers are not really set up for this 😄