Skip to content
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

Don't serialize ScriptGuid, UniqueId, and HistoryId #327

Merged
merged 6 commits into from
Jul 26, 2023

Conversation

Dekkonot
Copy link
Member

Closes #314 and resolves #324 for now.

I've decided that LuaSourceContainer.ScriptGuid, Instance.UniqueId, and Instance.HistoryId shouldn't serialize for the time being. All they do is make diff noise and they aren't especially useful for Rojo or Lune users at this time.

In the process, I discovered that rbx_reflector actually wasn't catching UniqueId properties so I've corrected that. I'll add it to my list of things to check in new data type implementations going forward.

Copy link
Member

@kennethloeffler kennethloeffler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept and execution looks ok to me...

Should this have a changelog entry in rbx_reflection_database?

Besides package compat, are there any other drawbacks to stripping these properties we should be aware of?

@Dekkonot
Copy link
Member Author

I was following in the precedent of not noting patches, but this is unusual and I suppose we should note it down somewhere. Good call.

Packages are the main issue with UniqueId and HistoryId, since that's all they're used for as of this moment. This was confirmed a few versions ago by the engineer who implemented them. While it's possible that's changed, I don't think it has.

With ScriptGuid, as far as I'm aware the only use for it is to preserve the IDE state for scripts. When you close a place in Studio, it saves what scripts you had open and any breakpoints you had for them, and it uses the GUID of scripts to do that. This technically means that we lose what little compatibility we had with the debugger, which would suck, but it only impacts people who were running Studio generated files through rbx-dom and then reopening them in Studio for debugging. Mind you, this also means that those same people had to not be using a workflow that duplicated ScriptGuid, since that'd also break it.

I think those are acceptable compatibility concerns because they're not really something we support beyond it being a coincidence.

@Dekkonot Dekkonot merged commit 5db11f8 into rojo-rbx:master Jul 26, 2023
2 checks passed
@Dekkonot Dekkonot deleted the no-unique-properties branch July 26, 2023 02:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants