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

Fix PatchTree incorrect changeList entries on decode failure #804

Conversation

kennethloeffler
Copy link
Member

@kennethloeffler kennethloeffler commented Oct 21, 2023

While I was testing #803, I discovered that PatchTree.build can create malformed changeList entries when a property decode fails. This causes errors when other parts of the system (such as DomLabel) attempt to interact with them. The core of the problem is that we're using next to obtain prop type-value pairs from the encoded property values in the incoming patch. next returns multiple values (in this case, the type followed by the value), which are then passed as-is to addProp or the table cons. What we want instead is to only pass the prop value.

This PR simply uses a select to only pass the prop value. I can also assign the result of next to variables in these cases if we think select is too ugly...

@Dekkonot
Copy link
Member

This seems like a fine patch to me, no need to store it in a variable.

@Dekkonot Dekkonot added the skip changelog PRs that may skip the changelog enforcement check label Oct 22, 2023
@Dekkonot Dekkonot enabled auto-merge (squash) October 22, 2023 23:57
@Dekkonot Dekkonot merged commit feac29e into rojo-rbx:master Oct 22, 2023
4 of 5 checks passed
@kennethloeffler kennethloeffler deleted the fix-patchtree-incorrect-decode-failure-change-list-entries branch October 23, 2023 02:41
Dekkonot pushed a commit to UpliftGames/rojo that referenced this pull request Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changelog PRs that may skip the changelog enforcement check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants