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

Add symbol for element status #49

Merged
merged 5 commits into from
Mar 18, 2018

Conversation

AmaranthineCodices
Copy link
Contributor

Super simple PR that fixes #20. Adds a symbol, Roact.Element, that is present in every table returned from Roact.createElement.

@coveralls
Copy link

coveralls commented Mar 14, 2018

Coverage Status

Coverage increased (+0.02%) to 87.43% when pulling 51bb81d on AmaranthineCodices:symbol-named-components into 965490b on Roblox:master.

@LPGhatguy
Copy link
Contributor

Does anything check isElement? You might remove that flag in the same PR and update anything that used it.

@AmaranthineCodices
Copy link
Contributor Author

It's never used in Roact; I've removed it. Should be good to go!

@LPGhatguy
Copy link
Contributor

One more note, should we expose this symbol?

If we want to start exposing these type markers, should we have them of the form

element = {
    [Core.isElement] = true,
}

or of the form

element = {
    type = Core.Element,
}

@AmaranthineCodices
Copy link
Contributor Author

It really should be exposed - that's the point of #20. I think it's probably best to store it with the symbol as the key just to avoid colliding with anything, but really it could go either way.

@LPGhatguy
Copy link
Contributor

I think we get better error messages if we key with type consistently, since then you can tell the user what kind of thing they mistakenly passed in. It does potentially collide with other systems a bit, but I think that's okay.

@AmaranthineCodices
Copy link
Contributor Author

We can't use type - there's already a naming collision in the Roact codebase >.<

roact/lib/Core.lua

Lines 115 to 119 in 340e013

local element = {
isElement = true,
type = elementType,
props = props,
}

@LPGhatguy
Copy link
Contributor

Hah!

I want to change that field to be component anyways. createElement is probably the oldest function in the Roact codebase.

@AmaranthineCodices
Copy link
Contributor Author

Should I change it to that then? :p

@LPGhatguy
Copy link
Contributor

Yeah!

@AmaranthineCodices
Copy link
Contributor Author

Should be good to go now, though this will cause merging problems with #50.

@LPGhatguy
Copy link
Contributor

Conflicts are now a go! 😅

@@ -37,7 +37,7 @@ local function isPortal(element)
return false
end

return element.type == Core.Portal
return element.component == Core.Portal
Copy link
Contributor

Choose a reason for hiding this comment

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

Eurgh, I can't wait to rewrite the reconciler + tests so that we have better coverage for refactors like this...

@LPGhatguy LPGhatguy merged commit c5e84b7 into Roblox:master Mar 18, 2018
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.

More reliable detection of Roact elements
3 participants