Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

Throw a nicer error when failing to set primitive properties #21

Merged
merged 5 commits into from
Jan 29, 2018

Conversation

AmaranthineCodices
Copy link
Contributor

This fixes #19. It adds a prettier error message (with stack trace shown if Roact.DEBUG_ENABLE has been called; stack trace is drawn from the new element, not the old one) when reconciliation tries to set a primitive element property to an invalid type (or anything that causes Reconciler._setRbxProp to throw).

Example stack trace

Roact.DEBUG_ENABLE has been called.

21:18:47.659 - Failed to set property Size on primitive instance of class Frame
21:18:47.660 - ReplicatedStorage.Roact.Reconciler:456: bad argument #3 to '?' (UDim2 expected, got number)
21:18:47.661 - 
21:18:47.661 - Stack Begin
21:18:47.662 - Script 'ReplicatedStorage.Roact.Core', Line 167 - field createElement
21:18:47.662 - Script 'Players.ProlificLexicon.PlayerGui.LocalScript', Line 13 - method render
21:18:47.664 - Script 'ReplicatedStorage.Roact.Component', Line 152 - method _forceUpdate
21:18:47.665 - Script 'ReplicatedStorage.Roact.Component', Line 127 - method _update
21:18:47.665 - Script 'ReplicatedStorage.Roact.Component', Line 114 - method setState
21:18:47.666 - Script 'Players.ProlificLexicon.PlayerGui.LocalScript', Line 21
21:18:47.666 - Stack End
21:18:47.667 - 
21:18:47.667 - Stack Begin
21:18:47.668 - Script 'ReplicatedStorage.Roact.Reconciler', Line 410 - field _reconcilePrimitiveProps
21:18:47.669 - Script 'ReplicatedStorage.Roact.Reconciler', Line 284 - field _reconcile
21:18:47.669 - Script 'ReplicatedStorage.Roact.Component', Line 156 - method _forceUpdate
21:18:47.670 - Script 'ReplicatedStorage.Roact.Component', Line 127 - method _update
21:18:47.670 - Script 'ReplicatedStorage.Roact.Component', Line 114 - method setState
21:18:47.671 - Script 'Players.ProlificLexicon.PlayerGui.LocalScript', Line 21
21:18:47.671 - Stack End

Remarks

I'm not happy with the code duplication, but extracting it into a function will add another line to the stack trace - is that something that's acceptable if it means eliminating the code duplication?

There are no tests for this behavior, like most of Reconciler - I'm not sure if lemur supports type-checks on Roblox properties at the moment. If it does I'll write some tests for this.

I've suppressed the line-number reporting as much as possible (read: not a lot) by using error(message, 0). This removes the initial line that reports the direct source of the error (...Roact.Reconciler:line#). Since this is an error raised by Roact, not a bug in Roact, I'm assuming we want to include as little information about Roact's internals to cut down on clutter.

@coveralls
Copy link

coveralls commented Jan 27, 2018

Coverage Status

Coverage decreased (-1.2%) to 84.468% when pulling fdb19d7 on AmaranthineCodices:nicer-primitive-throw into f0a9981 on Roblox:master.

@LPGhatguy
Copy link
Contributor

Neat!

Does it make sense to move this error catching into _setRbxProp itself? That would avoid adding another line of noise to the stack trace while still deduplicating the code a bit, but it might require shuffling _setRbxProp around to make it nice.

@AmaranthineCodices
Copy link
Contributor Author

I would have done it, but _setRbxProp doesn't know anything about the element that caused the property to be set - it only knows about the primitive. However, that would also allow removing the duplicate code in _reifyInternal, though.

I think that'd be worth it, all things considered. I'll change the PR in a few minutes.

Used in _setRbxProp to avoid creating a new closure for every property set.
]]

local function _rawSet(rbx, key, value)
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the allusion to rawset with this function name, but this version isn't very raw, is it? Would there be any problem with just naming this function set?

As a small style nitpick, you should remove the blank line after the documentation comment to group it visually with this function.

)

error(message, 0)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we introduce nice stack trace logic on the error cases below as well?

I haven't seen them ever come up in real code, but giving them equally pleasant messages would be good. I can understand if you'd want to push that into a different PR as well, though.

@AmaranthineCodices
Copy link
Contributor Author

Q: The Travis CI build is failing because line 449 is too long. Should I break the string across multiple lines or just suppress the line length warning for this line (-- luacheck: ignore 6)?

@LPGhatguy
Copy link
Contributor

I think suppressing the line length warning is okay in this case. After that, this looks good to me!

Do you think there will be any significant performance impacts here? This does introduce a pcall for every property set!

@AmaranthineCodices
Copy link
Contributor Author

AmaranthineCodices commented Jan 29, 2018

I ran some benchmarks using the same methodology as my blog post.

For comparison, here are results without this PR:

Total time for 1000000 iterations: 7.107126; avg. time: 0.000007
Total time for 1000000 iterations: 8.028523; avg. time: 0.000008
Total time for 1000000 iterations: 7.983292; avg. time: 0.000008
Total time for 1000000 iterations: 8.050372; avg. time: 0.000008
Total time for 1000000 iterations: 7.076552; avg. time: 0.000007
Total time for 1000000 iterations: 8.020197; avg. time: 0.000008
Total time for 1000000 iterations: 7.499279; avg. time: 0.000007
Total time for 1000000 iterations: 5.513306; avg. time: 0.000006
Total time for 1000000 iterations: 5.514822; avg. time: 0.000006
Total time for 1000000 iterations: 5.428669; avg. time: 0.000005

Here are the results with the PR applied:

Total time for 1000000 iterations: 9.542830; avg. time: 0.000010
Total time for 1000000 iterations: 8.182999; avg. time: 0.000008
Total time for 1000000 iterations: 8.200007; avg. time: 0.000008
Total time for 1000000 iterations: 8.326582; avg. time: 0.000008
Total time for 1000000 iterations: 8.379032; avg. time: 0.000008
Total time for 1000000 iterations: 7.557598; avg. time: 0.000008
Total time for 1000000 iterations: 7.252007; avg. time: 0.000007
Total time for 1000000 iterations: 5.779183; avg. time: 0.000006
Total time for 1000000 iterations: 5.696716; avg. time: 0.000006
Total time for 1000000 iterations: 5.717189; avg. time: 0.000006

Marginally slower, and the difference between the two scenarios gets faster over time (branch prediction? CPU-level caching? some Lua optimization that speeds up hot code paths? I have no idea). I think that's acceptable for this. If the overhead gets to be too much, one option is to only use pcall when DEBUG_ENABLE has been called, since the only purpose of this is for debugging.

@LPGhatguy
Copy link
Contributor

That performance gradient is super weird! Looks like performance should be just fine.

@LPGhatguy LPGhatguy merged commit 1d850f5 into Roblox:master Jan 29, 2018
@AmaranthineCodices AmaranthineCodices deleted the nicer-primitive-throw branch January 29, 2018 18:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make reconciliation errors more obvious
3 participants