validateProps #94

AmaranthineCodices · 2018-05-15T17:00:11Z

Closes #24. This is an integration of PropTypes in Roact.

TODO:

Gate type checks behind a configuration flag
Document this

This depends on #93.

coveralls · 2018-05-15T17:02:55Z

Coverage increased (+0.2%) to 90.374% when pulling 8f7d97b on AmaranthineCodices:propTypes into 077672b on Roblox:master.

LPGhatguy · 2018-05-15T18:09:30Z

I think the prop types implementation, no matter which one we use, should just have a functional interface to maximize the ability to support multiple implementations and custom validators. We probably don't need an apply or validate method -- just functions would work well!

I imagine the signature for a prop validator function would be (props, propKey, component) -> success, result.

Usage would still be:

Component.propTypes = {
    foo = PropTypes.number, -- or any function of the form above
}

AmaranthineCodices · 2018-05-16T01:42:31Z

Some further notes about this:

Hard dependency on Add stackTrace getter #93 in its current form, where the stack trace method is named rootElementTraceback.
Another stack trace is collected when the component is created; this is used to provide information about what component to look at when propTypes is not a function.
Compatible with any validation function that has the signature value -> success, reason.
Default traceback message has been moved from the reconciler to Core under the name _defaultSource. It may need a better name; I'm not sure!

AmaranthineCodices · 2018-05-23T16:43:36Z

This should be ready to go now!

LPGhatguy

I like where this is going and I'm super excited to get this feature in!

LPGhatguy · 2018-05-25T18:16:46Z

.gitmodules

@@ -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


Can we revert this change in this PR?

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 >.<

LPGhatguy · 2018-05-25T18:19:03Z

docs/api-reference.md

+```
+
+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.


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"

LPGhatguy · 2018-05-25T18:19:27Z

lib/Config.lua

@@ -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,


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

LPGhatguy · 2018-05-25T18:20:42Z

lib/Component.lua

+	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,


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

LPGhatguy · 2018-05-25T18:21:43Z

lib/Core.lua

@@ -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"


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. 🤔

LPGhatguy · 2018-05-25T18:24:00Z

lib/Component.lua

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


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

LPGhatguy · 2018-05-25T18:24:58Z

lib/Component.lua

+		-- 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(


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!

ZoteTheMighty · 2018-05-26T00:23:51Z

lib/Component.spec.lua

+			GlobalConfig.reset()
+		end)
+
+		it("should not be run if typeChecking is false", function()


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

Oops, yeah! Fixed now.

ZoteTheMighty · 2018-05-26T00:27:48Z

lib/Component.lua

+			typeof(validator),
+			self._extendTraceback),
+		0)
+	end


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.

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.

Gotcha, that makes sense!

LPGhatguy · 2018-05-29T19:11:46Z

An interesting thing that popped into my mind: does gating this behind a global flag make unit testing harder?

Naturally, you want to run all tests with typechecking enabled and also develop with typechecking enabled, but how do you best represent that with the config system only (rightfully?) letting you set configuration once?

AmaranthineCodices · 2018-06-05T22:58:56Z

I'm not entirely sure it blocks unit testing, to be honest, though that may be an artifact of how I run my unit tests. My tests don't run at the same time as the main entry point (they have their own entry point), so the configuration can be set once per entry point without issue.

WRT this: should we hold off on merging until after the new reconciler? I can migrate this to the new one once it's landed, or we can merge it now and reimplement validation.

LPGhatguy · 2018-06-18T20:32:56Z

lib/Component.lua

+
+	local validator = self.validateProps
+
+	if validator == nil then return end


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

LPGhatguy · 2018-06-25T18:24:46Z

I think we should push to get this merged; I'll cover any nitpicks I have either with commits to your PR branch directly or in commits afterwards.

I bet we can make this change really small to minimize the impact and make porting to any new reconciler a lot easier.

AmaranthineCodices · 2018-06-25T23:29:20Z

Alright! I've gotten this back to mergeable as of now; I think this should be pretty easy to port to the new reconciler!

AmaranthineCodices · 2018-12-18T02:19:08Z

Closing in favor of #174 targeted at the new reconciler.

AmaranthineCodices added 2 commits May 15, 2018 12:46

add proptypes dependency

47350e4

first draft

61614d4

AmaranthineCodices added 9 commits May 15, 2018 20:40

expunge PropTypes

567d583

refactor default source out of Reconciler and into Core

4f6d3ab

refactor to use new PropTypes API

136890b

gate behind a config flag

e1332c3

make tests work again

5f93de2

make defaultSource private

bb62d66

refactor tests so they work now

226c546

beef up test suite

15c8a78

further expand test suite

4f811d7

AmaranthineCodices added 4 commits May 16, 2018 17:56

update to newest version of Roblox#93

5802c1f

rename to validateProps

a59233d

add a default reason

6c8ae89

add to docs

8e411d9

AmaranthineCodices changed the title ~~WIP: PropTypes implementation~~ validateProps May 23, 2018

plug PropTypes

8f62b6e

LPGhatguy suggested changes May 25, 2018

View reviewed changes

AmaranthineCodices added 7 commits May 25, 2018 19:17

merge in

2c50417

rename config key to propValidation

74990f4

convert failureReason to string

255bc80

break config statement across multiple lines

4643508

tweak error message

f9d2594

rename _defaultSource to _defaultElementTracebackMessage

e7afda4

refactor docs

36df87c

ZoteTheMighty reviewed May 26, 2018

View reviewed changes

AmaranthineCodices added 2 commits May 25, 2018 20:25

squash warning

ca9cc17

fix test name

417d568

ZoteTheMighty reviewed May 26, 2018

View reviewed changes

LPGhatguy reviewed Jun 18, 2018

View reviewed changes

LPGhatguy and others added 3 commits June 25, 2018 14:11

Merge branch 'master' into propTypes

ca6e364

break statement across multiple lines

a600bac

fix tests

36cad55

AmaranthineCodices and others added 2 commits June 28, 2018 17:30

Merge branch 'master' into propTypes

8933cf0

fix mishap while merging

8f7d97b

AmaranthineCodices mentioned this pull request Dec 18, 2018

validateProps for new reconciler #174

Merged

1 task

AmaranthineCodices closed this Dec 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

validateProps #94

validateProps #94

AmaranthineCodices commented May 15, 2018 •

edited

Loading

coveralls commented May 15, 2018 •

edited

Loading

LPGhatguy commented May 15, 2018

AmaranthineCodices commented May 16, 2018

AmaranthineCodices commented May 23, 2018

LPGhatguy left a comment

LPGhatguy May 25, 2018

AmaranthineCodices May 25, 2018

LPGhatguy May 25, 2018

LPGhatguy May 25, 2018

LPGhatguy May 25, 2018

LPGhatguy May 25, 2018

LPGhatguy May 25, 2018

LPGhatguy May 25, 2018

ZoteTheMighty May 26, 2018

AmaranthineCodices May 26, 2018

ZoteTheMighty May 26, 2018

AmaranthineCodices May 26, 2018

ZoteTheMighty May 26, 2018

LPGhatguy commented May 29, 2018

AmaranthineCodices commented Jun 5, 2018 •

edited

Loading

LPGhatguy Jun 18, 2018

LPGhatguy commented Jun 25, 2018

AmaranthineCodices commented Jun 25, 2018

AmaranthineCodices commented Dec 18, 2018

		```

		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.


		local validator = self.validateProps

		if validator == nil then return end

validateProps #94

validateProps #94

Conversation

AmaranthineCodices commented May 15, 2018 • edited Loading

coveralls commented May 15, 2018 • edited Loading

LPGhatguy commented May 15, 2018

AmaranthineCodices commented May 16, 2018

AmaranthineCodices commented May 23, 2018

LPGhatguy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LPGhatguy commented May 29, 2018

AmaranthineCodices commented Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

LPGhatguy commented Jun 25, 2018

AmaranthineCodices commented Jun 25, 2018

AmaranthineCodices commented Dec 18, 2018

AmaranthineCodices commented May 15, 2018 •

edited

Loading

coveralls commented May 15, 2018 •

edited

Loading

AmaranthineCodices commented Jun 5, 2018 •

edited

Loading