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

Make Fragment an Element #214

Merged
merged 9 commits into from
Jun 20, 2019
Merged

Conversation

jeparlefrancais
Copy link
Contributor

@jeparlefrancais jeparlefrancais commented Jun 7, 2019

Closes #205

Fragments can now be used directly as children of a component:

local function component()
    return Roact.createElement("Frame", {}, {
        fragments = Roact.createFragment({
            A = Roact.createElement("Frame", {}, {})
        })
    })
end

Turning Fragments into Elements make them have their own virtual node, so they can be reconciled as any other nodes.

I'm not so sure about how to write good tests for that, let me know what's the best way (not sure where to assert exactly).

Checklist before submitting:

  • Added entry to CHANGELOG.md
  • Added/updated relevant tests
  • Added/updated documentation

Fragments can not be used directly as children of a component:

local function component()
    return Roact.createElement("Frame", {}, {
        fragments = Roact.createFragment({
            A = Roact.createElement("Frame", {}, {})
        })
    })
end

Turning Fragments into Elements make them have their own virtual node,
so they can be reconciled as any other nodes.
@ZoteTheMighty
Copy link
Contributor

For testing, I think it should be possible to use the mountVirtualNode method of the reconciler to get back a tree of virtual nodes that you can introspect to some degree? I'm not sure if you'll run into issues with data contained in private internal tables, but I don't think you will.

Look at some of the other tests in createReconciler.spec.lua for ideas, but I think the way to go is to get a tree of nodes and expect things like Type.of(node.currentElement) and hostKey and children, etc.

As for what cases to test, I'd just try combinations of places you can put fragments? An element with fragments in its children is a good start.

I'd also test things that will have name duplication and make sure they work out correctly, but those tests are more involved.

@jeparlefrancais jeparlefrancais marked this pull request as ready for review June 11, 2019 22:57
@ZoteTheMighty
Copy link
Contributor

Closing and re-opening to trigger CI checks.

@coveralls
Copy link

coveralls commented Jun 13, 2019

Coverage Status

Coverage decreased (-0.2%) to 93.012% when pulling b311ccb on jeparlefrancais:fragment-fix into b1db3f8 on Roblox:master.

return createElement("StringValue", {
Value = "B",
})
end)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these needs to be spies?

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 don't think they need, I could have asserted that they were called, but instead I removed them and asserted that the values are correctly created

Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

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

One more thing we should add is a changelog entry (this change is significant enough to warrant it). You should get an idea of what it would look like based on the existing entries (you can add it to the "Unreleased" section)

Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a comment

Choose a reason for hiding this comment

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

Other than my suggestion for rewording changelog, this looks good to me! If @LPGhatguy agrees, we should merge after the changelog fix!

CHANGELOG.md Outdated Show resolved Hide resolved
Co-Authored-By: Paul Doyle <37384169+ZoteTheMighty@users.noreply.github.com>
src/RobloxRenderer.spec.lua Show resolved Hide resolved
return createElement("StringValue", {
Value = "B",
})
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need these components at all?

If we do, we should define them using local function ChildA() ... end and such to line up with conventions for declaring function components.

src/createReconciler.spec.lua Show resolved Hide resolved
Copy link
Contributor

@LPGhatguy LPGhatguy left a comment

Choose a reason for hiding this comment

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

LGTM!

@LPGhatguy LPGhatguy merged commit 693e64e into Roblox:master Jun 20, 2019
@jeparlefrancais jeparlefrancais deleted the fragment-fix branch June 20, 2019 22:54
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.

Fragments don't work when used inline
4 participants