-
Notifications
You must be signed in to change notification settings - Fork 143
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went through and wrote down some thoughts as I went from top to bottom.
lib/Reconciler.lua
Outdated
if ref == nil then | ||
return | ||
local function DEBUG_warn(...) | ||
if DEBUG_LOGS then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if statement can be moved outside the function so that it isn't constantly evaluated. Once this if statement and the one below are moved out, then they can be combined since their condition is the same. The local DEBUG_warn can be set to an empty function if DEBUG_LOGS == false or to warn if DEBUG_LOGS == true
|
||
local Symbol = require(script.Parent.Symbol) | ||
|
||
local Type = newproxy(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the type.is function? Type.of(element) == Type.Element
would make it look nice next to the typeof(key) == "string"
. Perhaps adding "string", "boolean", "function" and what not to Type would also be a good idea. If we have a type enumeration, lets have all the types in there? It would be nice to use Type.of like so: Type.of(myVariable) == Type.String
. Using Type.of also allows developers to use types as a key in lookup tables without altering their syntax (otherwise myVariable[Type]
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about it more, Type.is does allow polymorphism without casts though. I wont remove the previous comment since it is good food for thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea, it would make the typechecks more in line with those using typeof
as well.
I don't think I want it to return any types except internal Roact ones, since for those you can just use typeof
and be more clear.
lib/Reconciler.lua
Outdated
]] | ||
function Reconciler.unmount(instanceHandle) | ||
local element = instanceHandle._element | ||
local function taskMountNode(details) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is details being constructed? I think the checks below should be done when details is constructed, and then the type of the details table can be set to Type.Details to signify that it conforms to the Details type structure, which type can be checked here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function that calls the task*
functions is the one creating the data, so I think doing the checks here (it's basically a constructor) is probably fine.
lib/Reconciler.lua
Outdated
assert(typeof(parentRbx) == "Instance" or typeof(parentRbx) == "nil") | ||
assert(typeof(isTreeRoot) == "boolean") | ||
|
||
return function(tree) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't finished reading the code, I'm just commenting as I'm going. That being said, if this is what a task looks like, I have a recommendation. A task could be a table with two things, a function to call, and the parameters to call it with. This had a couple advantages to a task being a function, all based upon the key difference of reusing a function. First, a function isn't created for every task. Second, the identity of the function doesn't change between tasks of the same type, allowing a system to do comparisons (think: prioritization or running same types of tasks one after another). Third, the task scheduler can see the parameters (I'm not sure how much of an advantage this is, but it is another degree of freedom).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first iteration of this was actually closer in line to that, where I had a task type as a string and a table of parameters with a giant switch
statement.
I think you're right that having function identity would be useful here, but I was a bit weary about allocating another table for every task. Maybe just mutating the input details
object to add a fn
member would be fine?
For most of the transforms related to data, I think we're going to need a lot more data, especially when it comes to deduplicating renders. That sort of stuff needs to be sorted top-to-bottom, so the generic task queue idea might not survive!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a function would be fine, although I'd feel better about assigning the description a task type. It seems to me to be the problem that types attempt to fix -- to decide which operations are valid for the given data, to allow comparisons between types, and to put a stamp of truth on specific characteristics (has the data it needs, of the right sub-types).
|
||
return typeof(value) == "table" and value[Type] == typeMarker | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the assigning of types to a table being done? There should be a single definition and a single function that verifies that a table type is valid. Since we have no static checking, we can either have greedy or lazy dynamic type verification. Since verifying types when a value is assigned can do too much checking, I'd recommend only verifying when needed. Greedy dynamic checking is valuable for debugging though since it allows the developer to find exactly when a type was invalidated. Perhaps a flag to switch between the two types? Greedy dynamic checking would have to be done by metatables or necessitate special setters for objects with types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also if we are using lazy evaluation with the Type.is function, then the type stored in the table becomes meaningless. The expected type is passed into the Type.is function and the given table may or may not match the type assigned to it. If we use Type.of or greedy evaluation, then the table's type would become meaningful again.
lib/Reconciler.lua
Outdated
taskIndex = 1, | ||
tasksRunning = false, | ||
connections = {}, | ||
mounted = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this tree truly isn't mounted yet, and if the root node fails to mount, this tree isn't ever truly mounted.
lib/Reconciler.lua
Outdated
tasks = {}, | ||
taskIndex = 1, | ||
tasksRunning = false, | ||
connections = {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these connections? I don't think I've seen them used yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, that was an oversight; we don't track a connection object with BindToRenderStep
, which is what I was anticipating.
lib/Reconciler.lua
Outdated
mount = mountTree, | ||
unmount = unmountTree, | ||
reconcileTree = reconcileTree | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall there is a lot of emphasis on task management in this file. While that is good, this file is going to get huge when the rest of the reconciler code is added in. I'd recommend moving the task oriented code into another file. The reconciler should create task types and associate priorities to them, which the task handler... handles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other PR I was working on, #102, does something like that with a Scheduler
object. Once I get a better grip on exactly how task scheduling is going to work, I'd definitely like to do that.
lib/Reconciler.lua
Outdated
return { | ||
mount = mountTree, | ||
unmount = unmountTree, | ||
reconcileTree = reconcileTree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tree is removed in the other two function names, but not reconcileTree. mount takes a node as an argument but for some reason its function is mountTree. unmount actually takes a tree, so I don't completely understand why tree was removed from its name. I'd almost say drop the "tree" in all these function names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a debug API, not entirely concerned with the outward semantics except that I can run example code against it!
lib/Reconciler.lua
Outdated
|
||
if not success then | ||
local source = element.source or DEFAULT_SOURCE | ||
return tree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rather than returning a new "tree" which contains internal state, it may not be a terrible idea to make an association table to turn the root node into the internal tree state. This would also fix the "tree" naming issues mentioned below, as all reconciler functions would just take the root node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's how Roact works today, and I think that was a mistake I'd like to rectify. A handle to a disjoint tree should be explicitly handled differently than a component instance!
… holding mutable render data
I think I still want to move tasks back into being objects.
Only mounting is ported to this system right now, I'll port the others in a moment>'
Current tension in the new reconciler is transitioning from what is currently This new idea is a lot less generic, but has the nice advantage that tasks at a given depth can be performed in any order, so we don't have to do as much data structure trickery. An open question is still how to schedule something to happen after a group of tasks without incurring significant overhead. With The extra complexity that React took on with fibers is starting to make a little bit more sense. We could do something similar with coroutines if we wanted to; maybe we should. |
setState suspension
… updates are introduced and the loop continues
… and messaging when users misuse setState
…urs several function call across several modules. Improves perf for mounting and updating leaf nodes
* add and test validateProps for new reconciler * add assertCalledWithDeepEqual to spies this is the ugliest name ever for this method, but idk what to call it * use assertCalledWithDeepEqual in validateProps testing * reorder validateProps invocation to after defaultProps + test * reindent validateProps tests * gate validateProps behind GlobalConfig setting
* Add messages to all asserts * Add flag to the test runner. Technically, none of our tests yet rely on the asserts (which is surprising), but we still want hem on for tests * Replace GlobalConfig functionality with something closer to the way we do debug stuff * Replace all DEBUG checks with new GlobalConfig approach * Remove the only-set-once logic from GlobalConfig * Remove public exposure of GlobalConfig's function, initialize config before tests
* Remove references to 'primitive' components/elements * Adjust some wording around kinds of components, fix broken link * functional component -> function component * Avoid usage of the verb 'reconcile' where not directly applicable * Deprecate Roact.reconcile in favor of Roact.update in master (#194) * Update CHANGELOG * Remove undocumented APIs `getGlobalConfigValue` and `Element` (#195) * Roact.reconcile replaced with Roact.update, uses of reconcile changed to update. Occasional mentions of 'the Roact reconciler' have been changed to simply 'Roact' * Add version info to new apis, more details to validateProps, added section for setGlobalConfig * Added the 'enable tracebacks' message to other places that try to use element tracebacks * Change misleading code in Roact.Change example (#196) * PR comments: mention deprecated names, fix links * Rename propertyValidation config key to propValidation, add code snippet for enabling it to docs
* Move virtual tree fields to internal sub-table * Minor code cleanliness feedback
* Test to verify ref.current == ref:getValue(), fix up docs * Update the 'Added in...' clauses in the api reference * update example I guess * A bit of touchup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.0 here we come!
Closes #127, closes #13, closes #146, closes #154, closes #158, closes #180, closes #24, closes #47, closes #48, closes #56, closes #58, closes #7, closes #8, closes #84, closes #88, closes #99.
This is an early PR that aims to rewrite almost every single line of code in Roact.
This PR is meant to serve as a synchronization point for a lot of things that I've wanted to refactor and fix about Roact, notably including most of the above issues. It's also a sprint to try to get Roact's test coverage to as close to 100% line and branch coverage as possible.
Missing Existing Features (Done!)
Docs
Roact.update
(formerlyRoact.reconcile
)New Features
New context API here?validateProps
port?Misc
currentElement
juggling