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

validateProps #94

Closed
wants to merge 30 commits into from
Closed
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
47350e4
add proptypes dependency
AmaranthineCodices May 15, 2018
61614d4
first draft
AmaranthineCodices May 15, 2018
567d583
expunge PropTypes
AmaranthineCodices May 16, 2018
4f6d3ab
refactor default source out of Reconciler and into Core
AmaranthineCodices May 16, 2018
136890b
refactor to use new PropTypes API
AmaranthineCodices May 16, 2018
e1332c3
gate behind a config flag
AmaranthineCodices May 16, 2018
5f93de2
make tests work again
AmaranthineCodices May 16, 2018
bb62d66
make defaultSource private
AmaranthineCodices May 16, 2018
226c546
refactor tests so they work now
AmaranthineCodices May 16, 2018
15c8a78
beef up test suite
AmaranthineCodices May 16, 2018
4f811d7
further expand test suite
AmaranthineCodices May 16, 2018
5802c1f
update to newest version of #93
AmaranthineCodices May 16, 2018
a59233d
rename to validateProps
AmaranthineCodices May 23, 2018
6c8ae89
add a default reason
AmaranthineCodices May 23, 2018
8e411d9
add to docs
AmaranthineCodices May 23, 2018
8f62b6e
plug PropTypes
AmaranthineCodices May 23, 2018
2c50417
merge in
AmaranthineCodices May 25, 2018
74990f4
rename config key to propValidation
AmaranthineCodices May 25, 2018
255bc80
convert failureReason to string
AmaranthineCodices May 25, 2018
4643508
break config statement across multiple lines
AmaranthineCodices May 25, 2018
f9d2594
tweak error message
AmaranthineCodices May 25, 2018
e7afda4
rename _defaultSource to _defaultElementTracebackMessage
AmaranthineCodices May 25, 2018
36df87c
refactor docs
AmaranthineCodices May 25, 2018
ca9cc17
squash warning
AmaranthineCodices May 26, 2018
417d568
fix test name
AmaranthineCodices May 26, 2018
ca6e364
Merge branch 'master' into propTypes
LPGhatguy Jun 25, 2018
a600bac
break statement across multiple lines
AmaranthineCodices Jun 25, 2018
36cad55
fix tests
AmaranthineCodices Jun 25, 2018
8933cf0
Merge branch 'master' into propTypes
AmaranthineCodices Jun 28, 2018
8f7d97b
fix mishap while merging
AmaranthineCodices Jun 28, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .gitmodules
Original file line number Diff line number Diff line change
Expand Up @@ -3,4 +3,4 @@
url = https://github.com/LPGhatguy/lemur.git
[submodule "modules/testez"]
path = modules/testez
url = https://github.com/Roblox/testez.git
url = https://github.com/Roblox/testez.git
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we revert this change in this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I...can try; this is an artifact of removing PropTypes. I'm not sure I'll be able to without borking the branch though. Submodules are weird >.<

24 changes: 23 additions & 1 deletion docs/api-reference.md
Original file line number Diff line number Diff line change
Expand Up @@ -304,4 +304,26 @@ end
```

!!! note
`getDerivedStateFromProps` is a *static* lifecycle method. It does not have access to `self`, and must be a pure function.
`getDerivedStateFromProps` is a *static* lifecycle method. It does not have access to `self`, and must be a pure function.

### validateProps
```
static validateProps(props) -> success, reason
```

Performs property validation. You can use this for type-checking properties; there is a [PropTypes](https://github.com/AmaranthineCodices/rbx-prop-types) library to assist in this.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should avoid calling props 'properties' except in their initial introduction. The (shortened) word is used intentionally to explicitly refer to the React and Roact concept as opposed to the general idea of properties.

When we introduce your library, let's say something like "there is a library called PropTypes that can be used to validate props declaratively"


This function will only be called if the `propertyValidation` configuration option is set to `true`. If this function returns `false`, the error message it returns will be thrown in the output, along with a stack trace pointing to the current element.

```lua
function MyComponent.validateProps(props)
if props.requiredProperty == nil then
return false, "requiredProperty is required"
end

return false
end
```

!!! note
`validateProps`, like `getDerivedStateFromProps`, is a *static* lifecycle method. It does not have access to `self`, and must be a pure function.
36 changes: 36 additions & 0 deletions lib/Component.lua
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ function Component:extend(name)
end

class.__index = class
class._extendTraceback = debug.traceback()

setmetatable(class, {
__tostring = function(self)
Expand Down Expand Up @@ -309,6 +310,10 @@ function Component:_forceUpdate(newProps, newState)
end
end

if newProps then
self:_validateProps(newProps)
end

if self.willUpdate then
self._setStateBlockedReason = "willUpdate"
self:willUpdate(newProps or self.props, newState or self.state)
Expand Down Expand Up @@ -374,6 +379,8 @@ function Component:_mount(handle)

self._setStateBlockedReason = "render"

self:_validateProps(self.props)

local virtualTree
if GlobalConfig.getValue("componentInstrumentation") then
local startTime = tick()
Expand Down Expand Up @@ -424,4 +431,33 @@ function Component:_unmount()
self._handle = nil
end

--[[
Performs property validation, if it is enabled.
]]
function Component:_validateProps(props)
if not GlobalConfig.getValue("propertyValidation") then return end
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep these types blocks to one statement per line, which I think makes them a little easier to read with how keyword-noisy Lua is:

if not GlobalConfig.getValue("propertyValidation") then
    return
end


local validator = self.validateProps

if validator == nil then return end
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put return on its own line? We try to follow one statement per line!

if typeof(validator) ~= "function" then
-- Hide as much as possible about error location, since this message
-- occurs from several possible call sites with different stack traces.
-- luacheck: ignore 6
error(("The value of validateProps must be a function (is a %q). Check the definition of the component extended at:\n%s"):format(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we tweak this error message?

"The value of validateProps must be a function, but it is a %s." ..
"\nCheck the component defined at:\n%s"

Maybe this is a case to bring in multilineError or whatever I've been calling it lately!

typeof(validator),
self._extendTraceback),
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.

Is this totally necessary? We don't bother doing this with lifecycle methods or things like defaultProps (making sure it's a table).

I can see the argument for doing it here since you're already opting into validation, just wondering what the thought process is since it's not something we do elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the time when validateProps is specified, it's not being written as a method:

function SomeComponent.validateProps(props)
    -- do some validation work
end

Instead, it's usually composed from a bunch of smaller validators, and the result of the composition is assigned. When you're using PropTypes it looks something like this:

SomeComponent.validateProps = PropTypes.object {
    someKey = PropTypes.number,
    someOtherKey = PropTypes.string,
}

The type check is an extra guard against validateProps being assigned a non-function, since it's probably not going to be declared as a method of the component class, but as a field of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, that makes sense!


local success, failureReason = validator(props)

if not success then
failureReason = failureReason or "<No failure reason was given by the validation function>"
error(("Property validation failed:\n%s\n%s"):format(
failureReason,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should explicitly call tostring on the failure reason in case the validator wants to express more detailed errors in some cases.

self:getElementTraceback() or Core._defaultSource), 0)
end
end

return Component
101 changes: 101 additions & 0 deletions lib/Component.spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ return function()
local Core = require(script.Parent.Core)
local Reconciler = require(script.Parent.Reconciler)
local Component = require(script.Parent.Component)
local GlobalConfig = require(script.Parent.GlobalConfig)

it("should be extendable", function()
local MyComponent = Component:extend("The Senate")
Expand Down Expand Up @@ -292,6 +293,106 @@ return function()
Reconciler.unmount(handle)
end)

describe("validateProps", function()
it("should be called if propertyValidation is set", function()
GlobalConfig.set({
propertyValidation = true,
})

local TestComponent = Component:extend("TestComponent")
local callCount = 0

TestComponent.validateProps = function(props)
callCount = callCount + 1
return true
end

function TestComponent:render()
return nil
end

local handle = Reconciler.mount(Core.createElement(TestComponent))
expect(callCount).to.equal(1)

handle = Reconciler.reconcile(handle, Core.createElement(TestComponent, {
foo = "bar",
}))
expect(callCount).to.equal(2)

Reconciler.unmount(handle)
expect(callCount).to.equal(2)

GlobalConfig.reset()
end)

it("should throw if the function returns false", function()
GlobalConfig.set({
propertyValidation = true,
})

local TestComponent = Component:extend("TestComponent")

TestComponent.validateProps = function(props)
return false
end

function TestComponent:render()
return nil
end

expect(function()
Reconciler.mount(Core.createElement(TestComponent))
end).to.throw()

GlobalConfig.reset()
end)

it("should throw if validateProps is not a function", function()
GlobalConfig.set({
propertyValidation = true,
})

local TestComponent = Component:extend("TestComponent")

TestComponent.validateProps = false

function TestComponent:render()
return nil
end

expect(function()
Reconciler.mount(Core.createElement(TestComponent))
end).to.throw()

GlobalConfig.reset()
end)

it("should not be run if typeChecking is false", function()
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the test name needs to be updated to refer to the config name propValidation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, yeah! Fixed now.

local TestComponent = Component:extend("TestComponent")
local callCount = 0

TestComponent.validateProps = function(props)
callCount = callCount + 1
return true
end

function TestComponent:render()
return nil
end

local handle = Reconciler.mount(Core.createElement(TestComponent))
expect(callCount).to.equal(0)

handle = Reconciler.reconcile(handle, Core.createElement(TestComponent, {
foo = "bar",
}))
expect(callCount).to.equal(0)

Reconciler.unmount(handle)
expect(callCount).to.equal(0)
end)
end)

describe("setState", function()
it("should throw when called in init", function()
local InitComponent = Component:extend("InitComponent")
Expand Down
2 changes: 2 additions & 0 deletions lib/Config.lua
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ local defaultConfig = {
["elementTracing"] = false,
-- Enables instrumentation of shouldUpdate and render methods for Roact components
["componentInstrumentation"] = false,
-- Enables type checking with the validateProps property on stateful components.
["propertyValidation"] = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Much like the recommendation in the docs, let's call this one propValidation to be more exact.

}

-- Build a list of valid configuration values up for debug messages.
Expand Down
3 changes: 3 additions & 0 deletions lib/Core.lua
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,9 @@ Core.None = Symbol.named("None")
-- Marker used to specify that the table it is present within is a component.
Core.Element = Symbol.named("Element")

-- The default "stack traceback" if element tracing is not enabled.
Core._defaultSource = "\n\t<Use Roact.setGlobalConfig with the 'elementTracing' key to enable detailed tracebacks>\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

This could probably stand to have a better name, especially now that it's left the scope of the reconciler! Maybe something like _elementTracebacksDisabledMessage?

I'm starting to feel that we should collect some of the error messages together into a module in a future PR. 🤔


--[[
Utility to retrieve one child out the children passed to a component.

Expand Down
8 changes: 3 additions & 5 deletions lib/Reconciler.lua
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ local Symbol = require(script.Parent.Symbol)

local isInstanceHandle = Symbol.named("isInstanceHandle")

local DEFAULT_SOURCE = "\n\t<Use Roact.setGlobalConfig with the 'elementTracing' key to enable detailed tracebacks>\n"

local function isPortal(element)
if type(element) ~= "table" then
return false
Expand Down Expand Up @@ -485,7 +483,7 @@ function Reconciler._setRbxProp(rbx, key, value, element)
local success, err = pcall(set, rbx, key, value)

if not success then
local source = element.source or DEFAULT_SOURCE
local source = element.source or Core._defaultSource

local message = ("Failed to set property %s on primitive instance of class %s\n%s\n%s"):format(
key,
Expand All @@ -504,7 +502,7 @@ function Reconciler._setRbxProp(rbx, key, value, element)
elseif key.type == Change then
Reconciler._singleEventManager:connectProperty(rbx, key.name, value)
else
local source = element.source or DEFAULT_SOURCE
local source = element.source or Core._defaultSource

-- luacheck: ignore 6
local message = ("Failed to set special property on primitive instance of class %s\nInvalid special property type %q\n%s"):format(
Expand All @@ -519,7 +517,7 @@ function Reconciler._setRbxProp(rbx, key, value, element)
-- Userdata values are special markers, usually created by Symbol
-- They have no data attached other than being unique keys

local source = element.source or DEFAULT_SOURCE
local source = element.source or Core._defaultSource

local message = ("Properties with a key type of %q are not supported\n%s"):format(
type(key),
Expand Down