WIP: Bindings #159

ZoteTheMighty · 2018-10-19T01:12:28Z

Closes #158

This PR is a work in progress and is built off of the new-reconciler branch

An initial implementation of Bindings. This addresses the basic use cases for it, and has some mechanisms to prevent unintended access to private functionality.

It also includes a very arbitrary example of how bindings can modify values outside of Roact's render in a more straightforward way than is possible with refs.

TODO:

Clean up any changes in unrelated files
Figure out plans for hiding private members of Bindings or not
Document Bindings feature
Add benchmarks, particularly for binding.map which may run every reconcile
Solicit/incorporate any feedback for Bindings API
Determine a mechanism to enable/disable the Bindings feature if needed

…w-rec

LPGhatguy

A handful of changes!

LPGhatguy · 2018-10-19T17:39:52Z

examples/start-examples.client.lua

@@ -1,4 +1,4 @@
-local PlayerGui = game:GetService("Players").LocalPlayer.PlayerGui
+local PlayerGui = game:GetService("Players").LocalPlayer:WaitForChild("PlayerGui")


An unfortunate consequence of APS, I guess: all my synchronization bugs in all the example code I write start coming to light.

lib/Binding.lua

LPGhatguy · 2018-10-19T17:44:54Z

lib/Binding.lua

+	Create a new binding object with the given starting value. This
+	function will be exposed to users of Roact.
+]]
+function Binding.create(initialValue)


One thing I've been doing lately is separating instance methods from static methods, since it becomes clearer which is which. Since we don't expose Binding to users, that's a little less important.

LPGhatguy · 2018-10-19T17:45:09Z

lib/Binding.lua

+		[Internal.changeSignal] = createSignal(),
+	}
+
+	binding[Internal.update] = function(self, newValue)


Can we define these once and then use metatables to get at them?

Yeah, that's probably the way to go. The thing that made this approach convenient is that I could use upvalues instead of making everything private via Symbols, which is an elegant solution conceptually but it makes the bodies of my functions kind of hideous.

lib/Binding.spec.lua

lib/createRef.spec.lua

LPGhatguy · 2018-10-19T18:05:59Z

lib/createSignal.lua

+]]
+
+local DEBUG_SIGNAL = true
+local debug_globalSubs = 0


Can we drop these?

Yeah, I intended to prune them from the final version

LPGhatguy · 2018-10-19T18:06:31Z

lib/init.lua


 	Change = require(script.PropMarkers.Change),
 	Children = require(script.PropMarkers.Children),
 	Event = require(script.PropMarkers.Event),
 	Ref = require(script.PropMarkers.Ref),

+	createBinding = Binding.create,


This should be grouped with the other methods!

LPGhatguy · 2018-10-19T18:06:57Z

rojo.json

-    "start-benchmarks": {
-      "path": "start-benchmarks.server.lua",
-      "target": "ServerScriptService.start-benchmarks"
-    },


When you're working locally, you can set the Disabled property on these scripts instead of removing them from your rojo.json config!

Ah, but that's only if the file exists :P I think the current state of new-reconciler might be missing the benchmark runner file:
thread '<unnamed>' panicked at 'Unable to watch partition start-benchmarks, with path C:\Users\pdoyle\dev\projects\refs-sample-project\modules\roact\start-benchmarks.server.lua! Make sure that it's a file or directory.', src\vfs\vfs_watcher.rs:86:25 note: Run with RUST_BACKTRACE=1 for a backtrace.

Do you want to revert this for the time being in this PR? I'll figure out whatever fun merge mishap happened that made us lose some of these things.

LPGhatguy · 2018-10-19T18:14:14Z

lib/Binding.lua

+	Creates a new binding from the given source and with the given mapping.
+
+	The source can either be a starting value or another Binding object,
+	which is useful for mapping a binding onto another binding


Can we make this function work only for creating a binding from another binding?

Alternatively, can we just inline this functionality into what map does?

Yeah this was where I was a bit conflicted trying to converge creating a binding vs. creating a mapped binding. I think moving the logic into map is probably reasonable.

Co-Authored-By: ZoteTheMighty <pdoyle@roblox.com>

…ject changes through setHotProperty

lib/RobloxRenderer.spec.lua

lib/createRef.spec.lua

LPGhatguy · 2018-10-25T20:17:40Z

rojo.json

-    "start-benchmarks": {
-      "path": "start-benchmarks.server.lua",
-      "target": "ServerScriptService.start-benchmarks"
-    },


Do you want to revert this for the time being in this PR? I'll figure out whatever fun merge mishap happened that made us lose some of these things.

lib/Binding.lua

LPGhatguy · 2018-10-25T20:19:22Z

lib/Binding.lua

+	local binding = Binding.create(mapFunc(self:getValue()))
+
+	binding._mapFunc = mapFunc
+	binding._source = self


We could probably use a more descriptive name, like __upstreamBinding?

LPGhatguy · 2018-10-25T20:19:50Z

lib/Binding.lua

+		This allows us to avoid subscribing to our source until someone
+		has subscribed to us, and avoid creating dangling connections
+	]]
+	if Type.of(self._source) == Type.Binding and self._disconnectSource == nil then


Is it sufficient to just check if self._source ~= nil here? It probably shouldn't be any other type!

LPGhatguy · 2018-10-25T20:21:14Z

lib/Binding.lua

+		_value = initialValue,
+		_changeSignal = createSignal(),
+		_mapFunc = mapIdentity,
+		_subCount = 0,


Let's list out other values we have here, even if they're nil, just so that we have a central place to look.

LPGhatguy · 2018-10-25T20:22:56Z

lib/Binding.lua

+	local binding = {
+		[Type] = Type.Binding,
+
+		_value = initialValue,


Do we want to group together all of the internal properties into a sub-table indexed by a symbol? I've been using this pattern in a couple places to hide values in a way that's a little more structured than this naming convention.

LPGhatguy · 2018-10-25T20:23:34Z

lib/Binding.lua

+end
+
+local Binding = {}
+local BindingInternal = {}


Can we rename this to prototype or bindingPrototype to make it more explicit what it does?

* hide private members in subtable * rename BindingInternal * check for nil instead of Type.of * rename 'source' to 'upstreamBinding' * list nil-initialized members in 'create' * directly return parent value from getValue

ZoteTheMighty added 15 commits October 17, 2018 10:54

Begin bindings example

4bda0ce

Fix rojo for my sanity

121f144

Basic prototype

eb5f241

Reimplement refs in tems of bindings

a5578c2

Temporarily fix ref example

0c5a15c

Add first-class-ref support

1ddcafd

Reconfigure towards multi-return

1744329

Touching up details

37ac343

Merge branch 'new-reconciler' into bindings-new-rec

37ed16d

Fix ref attach/detach stuff

d2eec60

Start adding tests t develop against

4fa27fc

more stable binding api, with protected update/subscribe methods

43c7a01

Merge remote-tracking branch 'origin/new-reconciler' into bindings-ne…

21cc9bd

…w-rec

Add todos,fix some naming

5da61e7

Add comments, revert ref example, slightly adjust bindings example

c544a69

LPGhatguy suggested changes Oct 19, 2018

View reviewed changes

LPGhatguy and others added 14 commits October 19, 2018 14:51

Style fix

a90e7ef

Co-Authored-By: ZoteTheMighty <pdoyle@roblox.com>

Remove unneeded newline

63078aa

Co-Authored-By: ZoteTheMighty <pdoyle@roblox.com>

Remove unneeded newline

c7e9c0a

Co-Authored-By: ZoteTheMighty <pdoyle@roblox.com>

Remove redundant return

523696e

Co-Authored-By: ZoteTheMighty <pdoyle@roblox.com>

Add more comments, switch signal to upvalue

75a29da

Additional testing

f15c377

More comments, rwork signal disconnection stuff a bit

e670653

Fix minor PR comments

3253ad4

Simplify, but remove privacy. Will try to reintroduced more sanely

5bc71a0

Made update and subscribe internal-only static functions; fixed refs

56ff246

Additional case to err on assigning directly to a ref

672adfb

Adjust ownership of binding logic in the renderer; funnel all host ob…

d63a61c

…ject changes through setHotProperty

Fully re-implement refs in the renderer; add tests

0dbf2ad

Add missing TODO, change type checking slightly

e86761e

LPGhatguy suggested changes Oct 25, 2018

View reviewed changes

ZoteTheMighty and others added 15 commits October 25, 2018 17:53

PR Comments:

2f7b5f5

* hide private members in subtable * rename BindingInternal * check for nil instead of Type.of * rename 'source' to 'upstreamBinding' * list nil-initialized members in 'create' * directly return parent value from getValue

PR Comments: update test

2339e82

Merge branch 'new-reconciler' into bindings-new-rec

a5051d9

Fix some naming conventions

f0326dc

Support tostring on createRef

7101b54

Revert unrelated file

b252a8d

Tidy up a handful of cases

849d143

Tinker with naming in RobloxRenderer

931e31c

Merge branch 'new-reconciler' into bindings-new-rec

f0b6505

Rename getDefaultPropertyValue

2e18383

Bikeshed

b1d9b77

Add note about checking instance properties

8ecc393

Remove trailing line-endings

5fd89cb

updater -> setter

bafcf4b

Sort and group imports

1bc1476

LPGhatguy merged commit 1bc1476 into Roblox:new-reconciler Oct 29, 2018

ZoteTheMighty mentioned this pull request Jan 2, 2019

New Reconciler: Document bindings #175

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Bindings #159

WIP: Bindings #159

ZoteTheMighty commented Oct 19, 2018 •

edited

Loading

LPGhatguy left a comment

LPGhatguy Oct 19, 2018

LPGhatguy Oct 19, 2018

LPGhatguy Oct 19, 2018

ZoteTheMighty Oct 19, 2018

LPGhatguy Oct 19, 2018

ZoteTheMighty Oct 19, 2018

LPGhatguy Oct 19, 2018

LPGhatguy Oct 19, 2018

ZoteTheMighty Oct 19, 2018

LPGhatguy Oct 25, 2018

LPGhatguy Oct 19, 2018

ZoteTheMighty Oct 19, 2018

LPGhatguy Oct 25, 2018

LPGhatguy Oct 25, 2018

LPGhatguy Oct 25, 2018

LPGhatguy Oct 25, 2018

LPGhatguy Oct 25, 2018

LPGhatguy Oct 25, 2018

		@@ -1,4 +1,4 @@
		local PlayerGui = game:GetService("Players").LocalPlayer.PlayerGui
		local PlayerGui = game:GetService("Players").LocalPlayer:WaitForChild("PlayerGui")

WIP: Bindings #159

WIP: Bindings #159

Conversation

ZoteTheMighty commented Oct 19, 2018 • edited Loading

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

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

ZoteTheMighty commented Oct 19, 2018 •

edited

Loading