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

Remove erroring SecurityCapabilities lua coder/decoder #372

Conversation

kennethloeffler
Copy link
Member

It looks like this coder/decoder was introduced in imitation of some other types, but the other types that have erroring implementations either:

  • Do not exist on any instance (Region3)
  • Are special-cased elsewhere downstream (Ref)
  • Wouldn't work anyway? (SharedString, although I think there is a case to be made that SharedString should NOT have an erroring lua coder/decoder)

SecurityCapabilities exists on all instances so an erroring implementation is inappropriate and causes problems for Rojo as per rojo-rbx/rojo#802. I think we can just remove the lua encoder/decoder, like how UniqueId simply doesn't have them either.

Copy link
Member

@Dekkonot Dekkonot left a comment

Choose a reason for hiding this comment

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

It does feel as though this is fixing a symptom rather than the underlying problem, since we should really not be sending properties we know aren't scriptable through Rojo to begin with and this wouldn't be a problem if we didn't. That said, this is also an issue so I'm okay with it.

In the future it may be worth looking into whether we can have a better mechanism for distinguishing between "unimplemented" and "unknown" for rbx_dom_lua, but that day is not today.

@Dekkonot Dekkonot merged commit 0e10232 into rojo-rbx:master Oct 20, 2023
1 of 2 checks passed
@kennethloeffler kennethloeffler deleted the no-securitycapabilities-lua-coder-decoder branch October 20, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants